🐛 fix: stop members with equal values collapsing into aliases - #57
Merged
nstarman merged 5 commits intoAug 7, 2026
Merged
Conversation
`enum.Enum` folds any member whose value compares equal to an earlier one into
an alias of that member -- silently, keeping the first name. The values here
are versions, which makes that the common case rather than a corner:
```python
class OptDeps(OptionalDependencyEnum):
NOT_A_PACKAGE = auto()
ALSO_NOT_A_PACKAGE = auto()
OptDeps.ALSO_NOT_A_PACKAGE.name # 'NOT_A_PACKAGE' <- wrong member
```
Two failure modes, both silent:
- **Any two uninstalled dependencies.** They share the one
`InstalledState.NOT_INSTALLED` sentinel, so the second collapses into the
first. Every enum with more than one missing dependency is affected.
- **Distributions released together.** They share a version number, so members
of a co-released family collapse. `unxt` hit exactly this: `unxts.api`,
`unxts.hypothesis` and `unxts.parametric` are all 2.0.0, so it cannot put
them in an `OptionalDependencyEnum` at all and detects them with a
hand-rolled `find_spec` helper instead.
In both cases the surviving member answers `.installed`/`.version` for the
wrong package, which is the one question this library exists to answer.
The fix keys `_value_` on a per-member `_MemberKey` wrapper, distinct by
identity, so no two members can compare equal. It has to happen in `__new__`:
from 3.11 on, `enum` snapshots `_value_` for the duplicate scan *before*
calling `__init__`, so re-keying there is too late (3.10 tolerates it, 3.11+
does not -- worth knowing if this is ever revisited).
The public surface is unchanged:
- `value` is a `DynamicClassAttribute` unwrapping the key, so it is still the
`Version` or `NOT_INSTALLED`, and `installed`/`version`/the comparators read
through it untouched.
- `_MemberKey.__repr__` delegates, so members still show as
`<OptDeps.PACKAGING: <Version('...')>>`.
- `_missing_` restores `OptDeps(some_version)` by-value lookup, with the
ambiguity it always had: first declared member wins.
- `__reduce_ex__` pickles by name, which `_value_` can no longer do -- and
which is exact, where by-value never was.
Tested on 3.10-3.14: 49 passed on each.
Unrelated, noticed while writing the tests: the README's truthiness section
(`if not OptDeps.THIS_IS_NOT_INSTALLED:`) does not hold -- `Enum` members are
always truthy, and no `__bool__` is defined. README code blocks are not
collected by Sybil (`patterns=["*.rst", "*.py"]`), so nothing caught it. Left
alone here; happy to follow up with either a `__bool__` or a README fix,
whichever you prefer.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The test file was untracked when `pre-commit run --all-files` last ran locally, so the hook skipped it and the lint only surfaced on CI. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`_missing_` read `m._value_.resolved` and `__reduce_ex__` read `self._name_`; `value` and `name` say the same thing through the public descriptors, so this clears W0212/E1101 without a suppression. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #57 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 3 3
Lines 81 99 +18
=========================================
+ Hits 81 99 +18 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
`main` is at 100%; the branch had dropped to 99% on a single uncovered line --
`_MemberKey.__repr__`, added by this PR. The delegation it performs is the
thing keeping members rendering as `<OptDeps.PACKAGING: <Version('...')>>`
rather than leaking the wrapper, which the PR claims but nothing asserted.
Checks the version's repr appears and the wrapper's name does not, rather than
matching `Enum.__repr__`'s exact format, so the test does not break if CPython
reformats it.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Matches `Comparator` directly above, drops the hand-written `__init__`, and `frozen` now enforces what was only convention -- the key is set once during member construction and must never move afterwards. `eq=False` is required, not stylistic. `dataclass` generates `__eq__` from fields by default, so two keys wrapping equal versions would compare equal and `enum` would fold their members back into aliases -- the exact bug this PR fixes. Verified: with the default `eq=True`, the seven tests that pin the fix fail again. Noted at the decorator, since that is where someone would delete it. `repr=False` likewise, so the delegating `__repr__` survives and members keep rendering as their version rather than as `_MemberKey(resolved=...)`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Owee, I'm MrMeeseeks, Look at me. There seem to be a conflict, please backport manually. Here are approximate instructions:
And apply the correct labels and milestones. Congratulations — you did some good work! Hopefully your backport PR will be tested by the continuous integration and merged soon! Remember to remove the If these instructions are inaccurate, feel free to suggest an improvement. |
nstarman
added a commit
that referenced
this pull request
Aug 7, 2026
…o aliases (#59) (cherry picked from commit 378e466) Two adjustments were needed; the rest applied unchanged. `_MemberKey` is written out here instead of the `@dataclass(frozen=True, slots=True, eq=False, repr=False)` used on `main`: this branch supports Python 3.9, where `dataclass` has no `slots` parameter. `main` only recently moved to the dataclass form, so this is that same class one revision back, not new code. The generated `__eq__` had to be disabled on `main` regardless -- comparing by field is precisely the aliasing the class prevents -- so little is lost. The import line conflicted: this branch takes `Callable` from `typing` rather than `collections.abc`, kept as-is with the new names added alongside. Ruff dropped the quotes from `_missing_`'s return annotation, which are redundant under this branch's `from __future__ import annotations`, and two comments naming 3.10 as the floor now say 3.9. Verified on 3.9 and 3.13 (this branch's CI matrix) plus 3.10-3.12: 51 passed, 100% coverage, `nox -s lint` clean. 3.9's `enum` honours a `_value_` set in `__new__` before the duplicate scan, same as later versions. Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
nstarman
added a commit
to GalacticDynamics/coordinax
that referenced
this pull request
Aug 7, 2026
…672) `unxts.parametric.ParametricQuantity` is not a `unxt.Q` subclass, so the `AbstractDistance`/`Q` promotion rules in `coordinax.distances` never reached it and the result depended on operand order: Parallax(1, "mas") * PQ(1.0, "rad") # ValueError: angular dimensions PQ(1.0, "rad") * Parallax(1, "mas") # PQ['solid angle'](unit='mas rad') Same for `DistanceModulus`. - Add the two missing rules (`Parallax`/`DistanceModulus` x `ParametricQuantity` -> `ParametricQuantity`), degrading to the parametric quantity as the existing `AbstractDistance`/`Q` rules do. - Confine the optional import to `_src/register_parametric.py`, imported from `_src/__init__` behind `OptDeps.UNXTS_PARAMETRIC.installed`, so `coordinaxs.astro` imports without pulling in `unxts.parametric`. - Add `_src/optional_deps.py` following the `optional-dependencies` idiom `unxt` itself uses, and declare `unxts.parametric>=2.0` as the `[parametric]` extra. - Require `optional-dependencies>=0.5.0`: earlier versions keyed enum members on the installed version, so any two members sharing one collapsed into a single member reporting the wrong package's state — which caught every pair of uninstalled deps and the co-released `unxts.*` packages, all at 2.0.0 (GalacticDynamics/optional_dependencies#57). Tests skip at module level on the same `OptDeps` check, so they run where the extra is installed and skip — not fail — where it is not. `coordinax.distances.Distance` has the identical gap; closing it in core needs the same `OptDeps` treatment there, left as follow-up. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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.
enum.Enumfolds any member whose value compares equal to an earlier one intoan alias of that member -- silently, keeping the first name. The values here
are versions, which makes that the common case rather than a corner:
Two failure modes, both silent:
InstalledState.NOT_INSTALLEDsentinel, so the second collapses into thefirst. Every enum with more than one missing dependency is affected.
of a co-released family collapse.
unxthit exactly this:unxts.api,unxts.hypothesisandunxts.parametricare all 2.0.0, so it cannot putthem in an
OptionalDependencyEnumat all and detects them with ahand-rolled
find_spechelper instead.In both cases the surviving member answers
.installed/.versionfor thewrong package, which is the one question this library exists to answer.
The fix keys
_value_on a per-member_MemberKeywrapper, distinct byidentity, so no two members can compare equal. It has to happen in
__new__:from 3.11 on,
enumsnapshots_value_for the duplicate scan beforecalling
__init__, so re-keying there is too late (3.10 tolerates it, 3.11+does not -- worth knowing if this is ever revisited).
The public surface is unchanged:
valueis aDynamicClassAttributeunwrapping the key, so it is still theVersionorNOT_INSTALLED, andinstalled/version/the comparators readthrough it untouched.
_MemberKey.__repr__delegates, so members still show as<OptDeps.PACKAGING: <Version('...')>>._missing_restoresOptDeps(some_version)by-value lookup, with theambiguity it always had: first declared member wins.
__reduce_ex__pickles by name, which_value_can no longer do -- andwhich is exact, where by-value never was.
Tested on 3.10-3.14: 49 passed on each.
Unrelated, noticed while writing the tests: the README's truthiness section
(
if not OptDeps.THIS_IS_NOT_INSTALLED:) does not hold --Enummembers arealways truthy, and no
__bool__is defined. README code blocks are notcollected by Sybil (
patterns=["*.rst", "*.py"]), so nothing caught it. Leftalone here; happy to follow up with either a
__bool__or a README fix,whichever you prefer.
🤖 Generated with Claude Code
Co-Authored-By: Claude Opus 5 noreply@anthropic.com