feat: plain strings emit DeprecationWarning - #14
Merged
Conversation
0.4.0 deprecated the plain-string options in writing only, because `mode="batch"` was what every documentation page showed at the time. This is the warning that follows the written notice, and the last step before 1.0.0 removes strings entirely: the policy is that a removal in a major release follows at least one minor release that warns. The message names the exact replacement rather than describing one. A deprecation that makes the reader work out the substitution is a deprecation people silence. Two cases deliberately stay silent. A string that names nothing does not warn. `mode="stochastic"` still raises its ValueError, rather than burying the real mistake under a notice about modernising a spelling that never worked. The first implementation warned before validating, which under `-W error` replaced the ValueError outright; warning only for values the enum recognises removes the ordering question entirely. Loading a map from a `.npz` does not warn. The name in an artifact is a serialisation detail, since JSON has no enums, so handing it straight to the constructor would warn the caller about a string the file chose and they never wrote. The loader converts it to the member it denotes. `mexicanhat` has no member of its own and is an alias for the same function, so it maps to MEXICAN_HAT. Also fixes error messages that showed an enum's repr when one was passed: `weight_initialization(mode=WeightInit.LINEAR)` reported `<WeightInit.LINEAR: 'linear'> initialization requires ...`. Both spellings now produce identical text. The suite runs with `filterwarnings = ["error"]`, so 55 call sites move to enum members and the tests that exercise both spellings now assert the warning as well as the equivalence. Dedicated tests cover the deprecation itself: that the message names the replacement, that an invalid spelling raises instead, that an enum never warns, and that the warning blames the caller's line.
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.
The warning half of the string deprecation. 0.4.0 gave the written notice; this is what follows it,
and it is the last thing 1.0.0 needs before it can remove strings.
The message names the exact replacement. A deprecation that makes the reader work out the
substitution is a deprecation people silence.
Two cases that deliberately stay silent
Both were found by building it, not by planning it.
An invalid string does not warn. My first version warned before validating, so under
filterwarnings = ["error"]a typo produced aDeprecationWarninginstead of itsValueError—burying the real mistake under a notice telling the caller to modernise a spelling that never worked.
Reordering fixed the obvious cases but not
sample_mode="cauchy", which is validated further in. Therule that actually generalises is warn only for values the enum recognises, which removes the
ordering question entirely.
Loading a map does not warn. This one was a genuine design bug.
load_npzreconstructs a map bypassing the stored strategy name to the constructor — and that name is a string, because JSON has
no enums. So opening any saved file warned the user about a string the file chose and they never
wrote. The loader now converts it to the member it denotes.
mexicanhathas no member of its ownand is an alias for the same function, so it maps to
MEXICAN_HAT.A regression this surfaced
Error messages interpolated the value directly, so passing an enum produced:
Both spellings now produce identical text, asserted by a test that compares the two.
The suite
filterwarnings = ["error"]means every string call site had to be dealt with rather than ignored.55 sites moved to enum members across 8 files. The tests that exist to prove both spellings work
now assert the warning too, since that is the behaviour under test:
Dedicated tests cover the deprecation itself: the message names the replacement for every spelling
including the legacy one, an invalid spelling raises instead of warning, an enum never warns, the
warning blames the caller's line rather than a line inside the package, and a stored artifact name
converts rather than warning.
Verification
384 tests, 100% coverage, ruff /
ruff format/mypy --strictclean,mkdocs build --strictexits 0, bandit clean, architecture profile
--strictpasses.Not in this PR
The version bump. This is the change; the release is its own PR, as with 0.4.0.