fix(core): name the file and key in config value type errors - #48
Merged
Merged
Conversation
Each config file's values are now type-checked before merging, so a wrong type reports its full key and file instead of a generic serde error from the merged config.
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.
Closes #46
validate_keysincrates/core/src/config.rsnow type-checks each known value in each config file before merging. It deserializes the value into the same type itsConfigfield uses, so the accepted set is unchanged. A wrong type reports`<key>` in <file> must be <expected>, where the expected text is "a non-negative integer or null" for global and per-language limits, "a non-negative integer" forhook.max_blocks, and "a list of strings" forignore,tests.patterns, andtests.exempt. The final deserialize of the merged config stays as a backstop.docs/spec.mdstates the rule, and the release draft gains a line for #46.One behavior change goes beyond the message. Main type-checked only the merged config, so a bad value in the user config was silently dropped when the repo config replaced the same
languages.<name>entry (QA 01 exits 0 on main). Each file is now checked on its own, the same way unknown keys already were, so that config now exits 2 and names the user file.hook.max_blocksof -1 and null;ignoreas a string or null; mixedtests.patterns; stringtests.exempt; non-stringtests.exemptentries[1]and["lines",1]; bad value in the repo config with a valid user configstring_global_limit_names_key_and_config_path,string_language_limit_names_full_path,float_limit_is_rejected,negative_max_blocks_names_key_and_config_path,string_ignore_is_rejected_as_non_list,non_string_test_pattern_is_rejected,string_test_exempt_is_rejected_as_non_list,non_string_test_exempt_entry_gets_list_message; QA 01 to 12, 17hook.max_blockscrates/core/src/config.rsnullglobal and per-language limits, zero limits,max_blocks0, validignore, empty config, unknown limit key with a bad value, non-object per-language limits,tests.exemptof["depth"]null_limits_still_loadand existing config tests; QA 13 to 16, 18 to 20cargo test --workspace --locked --all-targets,cargo clippy ... -D warnings,cargo fmt --check,cargo run -- check crates,cargo llvm-cov --fail-under-lines 94(94.92%),pickcheck check --base origin/main --fail-on new,worsened,unmatched(0 violations), on b48c1c5QA: a CLI pass ran release builds from main (213fbbd) and this branch over a scratch Git repo in 20 scenarios, with a user config supplied through
XDG_CONFIG_HOME, plus--changedand--base HEAD --format jsonwith a string limit. The 13 wrong-type cases now name the key and file, and the 7 valid-config and other-error cases are byte-identical to main. After the review fix, thetests.exemptcases were rerun:[1]now gets the list message instead ofunsupported tests.exempt key `<non-string>`, while["depth"]keeps its old message. The evidence stays local and outside the repo.The coverage floor stays at 94, which is 94.92% rounded down.
Review: independent xhigh reviews by Astra and Fable at d2c873b. Astra found nothing. Fable found no blockers. Its minor finding was that a non-string
tests.exemptentry kept the older<non-string>message. Its nits were a needless clone in the type check and a test that bundled three scenarios. b48c1c5 fixes all three, and the Fable recheck confirmed it. A nit to unify backtick style with the older shape messages was declined, because the new messages follow the existing unknown-key style and restyling older messages is outside this issue.