chore: inline needlessly extracted single-use helpers - #14048
Merged
Conversation
thaJeztah
previously approved these changes
Aug 14, 2026
Comment on lines
+104
to
+112
| func setServiceScale(project *types.Project, name string, replicas int) error { | ||
| service, err := project.GetService(name) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| service.SetScale(replicas) | ||
| project.Services[name] = service | ||
| return nil | ||
| } |
Member
There was a problem hiding this comment.
Looks like this is used in two places, but possibly even worth considering inlining it in both places;
Contributor
Author
There was a problem hiding this comment.
I would rather keep this one: it has two real callers (applyScaleOpts in create.go and runScale), and the value-semantics dance (GetService → SetScale → write back into project.Services[name]) is easy to get wrong — forgetting the write-back is a silent no-op. Before this PR runScale had exactly that duplication, drifting from the helper. Real reuse is the boundary this PR tries to preserve; happy to inline both if you feel strongly about it.
Member
There was a problem hiding this comment.
Ah, right yeah, the project.Services[name] = service may be easily overlooked. Yup, that's fair, no problem!
Member
There was a problem hiding this comment.
(I was actually looking; couldn't we just iterate over project.Services, but there was also "disabled services" etc to take into account).
Inline small functions that were extracted from their single call site without a real boundary to justify it — no reuse, no dedicated test, no responsibility of their own — so each caller now tells its whole story top-down: - getExecTarget, attachContainer, logContainer: one-line trampolines to getSpecifiedContainer / doAttachContainer / doLogContainer, which other call sites already use directly - removeImage: single-statement wrapper, unlike its removeVolume sibling which has actual logic - checkSelectedServices: named like a validation, actually a filter; the subtle rule (an unknown service is only an error with an explicit compose file) now reads where options.Services is rewritten - prepareLabels: mutated the map it received while looking pure at the call site; the label writes are now visible in getCreateConfigs - setDefaultTarget: mutation-by-pointer of the loop copy, now visible in the loop of injectFileReferences - buildVolumeOptions/buildTmpfsOptions/buildImageOptions: nil-guard + field copies; the buildMountOptions switch now shows side by side what each mount type propagates (buildBindOption keeps real logic and keeps buildMountOptions under the gocyclo limit) - displayDryRunBuildEvent: was longer than its only caller - hasMore: read like a predicate, was a one-line spinner restart - escapeDollarSign: wrapped a single bytes.ReplaceAll - extractEnvCLIDefined: replaced by the canonical compose-go helper types.NewMappingWithEquals().ToMapping(), as run.go already does - isPullPolicyValid: rebuilt the valid-values slice on every call; now a package-level list checked at the call site - viewFromStackList: projection now sits next to the render closure that consumes exactly its three fields No behavior change; single gocyclo threshold untouched. Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
validateNavigationMenu validated nothing: it resolves the TTY / COMPOSE_MENU / --menu precedence and mutates opts.navigationMenu — rename to resolveNavigationMenu. runScale duplicated setServiceScale's GetService/SetScale/write-back dance inline; move the helper next to its natural home in scale.go and use it from both callers. Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
ndeloof
force-pushed
the
cleanup-needless-extractions
branch
from
August 14, 2026 17:05
0077e76 to
a5992f6
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Systematic "extract method" helps nobody when the extracted function has a single caller, no test, and no responsibility of its own: the reader jumps around instead of reading the flow top-down. This PR inlines the small helpers where the extraction had no real boundary — actual reuse, testability or a responsibility seam — and keeps everything that has one.
Inlined (single caller, no dedicated test, body was a plain slice of its caller's flow): one-line trampolines that other call sites already bypass (
getExecTarget,attachContainer,logContainer,removeImage); helpers whose name hid a mutation or promised something else (prepareLabelsmutated the map it received,checkSelectedServicesfiltered rather than checked,hasMoreread like a predicate but restarted a spinner); and chunks whose name added nothing over the code itself (displayDryRunBuildEvent— longer than its caller,escapeDollarSign,buildVolume/Tmpfs/ImageOptions,viewFromStackList,extractEnvCLIDefined→ canonicaltypes.NewMappingWithEquals().ToMapping(),isPullPolicyValid→ package-level list +slices.Contains).Renamed/shared instead of inlined:
validateNavigationMenu→resolveNavigationMenu(it resolves and mutates, validates nothing);setServiceScalemoved to scale.go and now used byrunScale, which duplicated its four lines inline.Deliberately untouched: the
xxxCommand/runXxxcobra idiom (uniform across the package), symmetric families (toBake*,ensure*Down, …), callbacks/iterators, conversions with a real seam, everything protected by the gocyclo 16 threshold (prepareRunsits exactly at 16), andprintEvent's color switch — it differs fromcolorFnonapi.Working(SuccessColor vs nocolor), so unifying them would change the display.No behavior change;
golangci-lint(incl. gocyclo) and unit tests pass.