feat: document and enforce eager validation of named options (#310)#314
Open
seabbs-bot wants to merge 1 commit into
Open
feat: document and enforce eager validation of named options (#310)#314seabbs-bot wants to merge 1 commit into
seabbs-bot wants to merge 1 commit into
Conversation
Any function that accepts a set of named options -- keyword arguments, a scenario/backend registry, a set of sweep axes -- should validate them eagerly and, on an unrecognised name, raise an error naming every offending key and listing the valid set. A silently-ignored option name is a latent bug: a caller believes they set a value when they did not, and the mistake only surfaces (if at all) as a wrong result far from its cause. The practice already existed here as an exemplar (scaffold's SUPPORTED_LICENSES check) but was undocumented and unenforced. Add `test_option_validation(f, valid; n, rng)` (src/quality.jl): fuzzes a validating function `f` with random names outside `valid` and asserts each call throws, naming the rejected value and listing every valid entry. A package points it at each option-accepting entry point (e.g. `test_option_validation(k -> configure(; Dict(k => true)...), VALID_KEYS)`) instead of auditing by hand. Extract the exemplar into a named, directly testable helper (`_validate_license`, src/scaffold.jl) so `test/qa.jl` can dogfood the fuzz helper against the kit's own real check, not just a synthetic one. Document the convention on the test-infrastructure docs page, citing the exemplar and the new helper. `Random` becomes a new dependency of the main package and test environment (the fuzz helper's default `rng`). Co-authored-by: Sam Abbott <contact@samabbott.co.uk>
Contributor
Try this Pull Request!Option 1: Julia Package ManagerOpen Julia and type: import Pkg
Pkg.activate(temp=true)
Pkg.add(url="https://github.com/EpiAware/EpiAwarePackageTools.jl", rev="fix/310-eager-option-validation")
using EpiAwarePackageToolsOption 2: Local CheckoutIf you have the repo locally: git checkout fix/310-eager-option-validation
julia --project=. -e "using Pkg; Pkg.instantiate()" |
Contributor
|
📖 Documentation preview is ready! View the docs for this PR at: https://EpiAware.github.io/EpiAwarePackageTools.jl/previews/PR314/ This preview will be updated automatically when you push new commits. |
Codecov Report❌ Patch coverage is
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
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.
Summary
Fixes #310. Part of the #307 shared-quality-tooling epic.
Any function that accepts a set of named options -- keyword arguments,
a scenario/backend registry, a set of sweep axes -- should validate
them eagerly and, on an unrecognised name, raise an error naming every
offending key and listing the valid set. A silently-ignored option name
is a latent bug: a caller believes they set a value when they did not,
and the mistake only surfaces (if at all) as a wrong result far from
its cause. The worst case is a sweep axis, where a mistyped name can
send a whole run down the wrong path and only become visible deep
inside it.
The practice already existed here as an exemplar (
scaffold'sSUPPORTED_LICENSEScheck) but was undocumented and unenforcedelsewhere.
What changed
test_option_validation(f, valid; n = 50, rng = Random.default_rng())(
src/quality.jl): fuzzes a validating functionfwithnrandomnames outside
validand asserts each call throws, with a messagenaming the rejected value and listing every entry of
valid. Apackage points it at each option-accepting entry point, e.g.
test_option_validation(k -> configure(; Dict(k => true)...), VALID_KEYS),rather than auditing by hand.
(
_validate_license,src/scaffold.jl, behaviour-preserving) sotest/qa.jldogfoods the fuzz helper against the kit's own realcheck, not just a synthetic one.
test-infrastructure docs page,
citing the exemplar and the new helper, and noting it is wired in
per-entry-point rather than being part of the generic
quality.jltestset.
Randombecomes a new dependency of the main package and the testenvironment (the fuzz helper's default
rng).Test plan
task test: full suite green (2524 pass / 8 broken(xfail) / 0 fail)test_option_validationtestset intest/qa.jl: aconforming validator passes; a silent-accept validator, a
message-omits-the-key validator, and a message-omits-a-valid-entry
validator each correctly fail; a
String-flavoured valid set works;the real
_validate_license/SUPPORTED_LICENSESexemplar passesisolated formatting env, full suite (matches exactly), and quality
gates (Aqua/ExplicitImports/JET/doctests/docstrings/README/extension
ambiguities) all pass
This was opened by a bot. Please ping @seabbs for any questions.