Validate flags/positionals before the commands/nodes that own them - #629
Open
hexonal wants to merge 1 commit into
Open
Validate flags/positionals before the commands/nodes that own them#629hexonal wants to merge 1 commit into
hexonal wants to merge 1 commit into
Conversation
Context.Validate() iterated c.Path in trace order and called each element's Validate() as encountered. Since a command's own Path entry is appended before its flags' entries (flags are parsed after the command name in the token stream), a command's Validate() ran before its flags' Validate() - so a command validator couldn't rely on its own flags already being validated. A node's own flags/positionals always appear on the path right after that node, before the next node entry. So each node/command's own validation is now deferred until that boundary (or the end of the path) is reached, running it right after its own flags/positionals rather than before them. This only reorders each node relative to its own directly- owned flags; the relative order between different nodes on the path (e.g. a parent command still validates before its child command) is unaffected. Fixes alecthomas#611.
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
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.
Fixes #611.
Bug
Context.Validate()iteratesc.Pathin trace order and calls each element'sValidate()as it's encountered. Since a command's ownPathentry is appended before its flags' entries (flags come later in the token stream, e.g.command --flag value), the command'sValidate()ran before its flags'Validate()— so a command's validator couldn't rely on its own flags already being validated:Fix
A node's own flags/positionals always appear on the path contiguously right after that node, before the next node entry. So each node's own validation is now deferred (via a single
pendingNodevariable) until that boundary — or the end of the path — is reached, running it right after its own flags/positionals instead of before them.This only reorders each node relative to its own directly-owned flags. The relative order between different nodes on the path is unaffected — a parent command still validates before its child command, exactly as before. (An earlier version of this fix used a flat two-pass split over the whole path instead, which had a wider side effect: an ancestor command's validation could get delayed by an unrelated descendant command's flags. This version avoids that by only ever holding back the most recently encountered node, not the whole path.)
Known limitation
Resolve()appends resolver-derived flag values (e.g. from a config-fileResolver) to the end ofc.Pathwithout recording which node each one belongs to. Because of that, a node whose flags come from aResolver(rather than the command line) can still end up deferred past other nodes' resolver-supplied flags too — noted in a code comment. Fixing this properly would mean also threading ownership throughResolve()'s appended entries, which felt like more than this fix should take on; happy to follow up separately if that's worth doing.Tests
TestValidateCmdRunsAfterItsFlags— the issue's exact repro (single command, one flag).TestValidateNestedCmdRunsAfterOwnFlagsOnly— two-level nesting (app flag, parent command + its own flag, child command + its own flag), confirming each level's own flag validates immediately before it while app → parent → child ordering is preserved.Both confirmed to fail against pre-fix code and pass with the fix.