refactor(precedence): absorb firstNonEmpty helper variants into one precedence.First enforcer (links-va-001-precedence-jg6.1)#207
Merged
Conversation
…recedence.First enforcer (links-va-001-precedence-jg6.1) Three surviving helper variants (templates.firstNonEmpty, cli.firstNonEmptySyncBranch, cli.firstNonEmptySyncRemote — the latter two byte-identical) collapse into internal/precedence.First. The epic's proposed trim bool is deliberately not implemented: every sync candidate is already trimmed where it is produced, and template content must never be trimmed — so trim is a property of the value at its production boundary (the PathSpec precedent), not a mode of resolution. [LAW:no-mode-explosion] [LAW:single-enforcer] Helper-specific sync test becomes the precedence package test matrix. The other three filed sites (config.first, workspace.firstNonEmptyTrimmed, error_output.firstNonEmptyString) were already absorbed by #202-#204.
There was a problem hiding this comment.
Z.ai Coding Agent Review
Clean refactoring consolidating three duplicate firstNonEmpty helpers into a single precedence.First. No LAW violations introduced. The "trim at source, not at resolution" design is correct — all callsites trim their candidates before passing them. The new test suite covers the removed tests' contract plus edge cases. No changes requested.
✅ Approved
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
1 participant
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.
Summary
Closes out epic
links-va-001-precedence-jg6(F-14, Pattern C — mode flags hidden in helper-function variants).Of the six filed sites, three were already absorbed as side effects of #202–#204 (
config.first,workspace.firstNonEmptyTrimmed,error_output.firstNonEmptyString). The three survivors —templates.firstNonEmpty,cli.firstNonEmptySyncBranch,cli.firstNonEmptySyncRemote(the sync pair byte-identical under two names) — collapse into one enforcer:internal/precedence.First.Design deviation from the epic sketch (with reason, family precedent 4-for-4)
The filed sketch proposed
Precedence{candidates []string; trim bool}. Not implemented as filed, because atrim boolis itself the Pattern-C mode flag this epic exists to kill ([LAW:no-mode-explosion]), and inspection shows the trim distinction is illusory:validatedUpstreamRemote,singleRemote,debugOverride,defaultBranch) is alreadyTrimSpace'd where it is produced; the trim inside the helpers was duplicate enforcement.So trim is a property of the value at its production boundary (the PathSpec trim-at-construction precedent,
[LAW:single-enforcer]), and resolution is mode-free: variability lives in the values ([LAW:dataflow-not-control-flow]). Behavior at every callsite is byte-identical to before.Tests
precedencepackage test matrix pins the contract, including the deliberate "whitespace is a value, not emptiness" semantics.TestFirstNonEmptySyncBranchFollowsDeterministicPrioritydeleted — it asserted the removed structure ([LAW:behavior-not-structure]); its cases live in the matrix.go vet ./...and fullgo test ./...clean.