fix: use ArgMatches::value_source to distinguish user-provided from default#25
Open
amaksimo wants to merge 1 commit into
Open
fix: use ArgMatches::value_source to distinguish user-provided from default#25amaksimo wants to merge 1 commit into
amaksimo wants to merge 1 commit into
Conversation
amaksimo
force-pushed
the
add-regression-tests-for-arg-validation
branch
from
77f2d96 to
99e66f4
Compare
amaksimo
changed the title
amaksimo
force-pushed
the
add-regression-tests-for-arg-validation
branch
from
99e66f4 to
c0acad0
Compare
…efault Root-causes the class of bug fixed in efe936b. Previously validate_delimited_options used `source.field.is_some()` to mean "user passed this flag", which silently breaks the moment any delimited field gains a `default_value` — exactly what caused the parquet regression. Switches the check to `ArgMatches::value_source(..) == CommandLine`, which is clap's canonical "was this flag provided on argv?" query. Defaults are now safe to add on any field without breaking validation. - Adds `DELIMITED_OPTION_FLAGS` so adding a new delimited option is a single-line registration next to where it needs to be registered. - Matrix test covers {csv, tsv, parquet} x each delimited flag. - `defaults_do_not_count_as_user_provided` pins the core invariant at the validation function, independent of which fields happen to carry defaults today. Verified by temporarily adding `default_value = "\""` back to --quote: all tests still pass (the bug cannot be reintroduced via defaults).
amaksimo
force-pushed
the
add-regression-tests-for-arg-validation
branch
from
c0acad0 to
1609641
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
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
Root-causes the class of bug fixed in efe936b (removing
default_value = "\""from--quote). That fix was correct but fragile: it only addressed the one flag, and imposed an invisible "don't add defaults toSourceArgs" restriction that no one would know about.This PR removes the restriction by fixing the underlying validation logic.
Before
validate_delimited_optionsusedsource.field.is_some()to mean "user passed this flag". For anOption<String>with adefault_value, clap populates the field — so.is_some()is true even when the user passed nothing. That's what caused parquet loads to fail when--quotehad a default.After
Uses
ArgMatches::value_source(..) == ValueSource::CommandLine, which is clap's canonical answer to "did this flag appear on argv?". Defaults are now safe to add on anySourceArgsfield without breaking validation.DELIMITED_OPTION_FLAGSso registering a new delimited option is a single-line change in one place.{csv, tsv, parquet} × each delimited flag.defaults_do_not_count_as_user_providedpins the core invariant at the validation function, independent of which fields happen to carry defaults today.Test plan
cargo fmt -- --checkcargo clippy --all-targets --all-features -- -D warningscargo test --bin aurora-dsql-loader— 3/3 passdefault_value = "\""back to--quote; all tests still pass (the bug cannot be reintroduced via defaults). Reverted.