alunduil / alunduil/zfs-replicate
Snapshot equality returns NotImplemented for a non-Snapshot operand
- Langage dominant
- Python
- Étoiles
- 24
- Forks
- 6
- Merge moyen
- 3 h 11 min
- PR mergées (30 j)
- 49
Description
## Summary
`Snapshot.__eq__` yields the `NotImplemented` singleton when handed something
that isn't a `Snapshot`, so comparing a snapshot to an unrelated object
evaluates to `False` instead of raising.
## Current behaviour
`zfs/replicate/snapshot/type.py` guards the comparison with
`if not isinstance(other, Snapshot): raise NotImplementedError`, so any
comparison against another type crashes:
```pycon
>>> snapshot == "snap"
NotImplementedError
>>> snapshot in ["snap", 42]
NotImplementedError
```
The `if other is None: return False` guard above it exists only to keep
`== None` from reaching that raise.
## Motivation
Python's rich comparison protocol asks a method that doesn't recognise its
operand to return the `NotImplemented` singleton. The interpreter then tries
the reflected operation and falls back to identity, which produces `False`.
`NotImplementedError` is a different object: an exception class, meant for an
abstract method that a subclass has to fill in. Raising it turns an ordinary
comparison into a crash, and any `in` over a sequence holding mixed types hits
it.
Nothing in the tree catches `NotImplementedError`, so the raise has no
consumer today. Surfaced while working #502 and left out of that pull request
as a behaviour change beyond its scope.
## Approach and alternatives
Return `NotImplemented` in place of the raise. That subsumes the `None` guard:
`None` isn't a `Snapshot`, and the interpreter's identity fallback already
answers `False`, so both guards collapse into one.
Considered and set aside: keep the raise and document it as a deliberate
contract. Nothing depends on the exception, and a type the interpreter can't
compare without crashing is a hazard for any caller that puts snapshots in a
mixed sequence.
## Scope
`zfs/replicate/snapshot/type.py`,
`zfs_test/replicate_test/snapshot_test/type_test.py`
## Out of scope
Agreement between `__eq__` and `__hash__`, which #502 carries. The
suffix-matching semantics the guard sits in front of, settled by #390 and #439.
## Acceptance criteria
- [ ] `snapshot == ` evaluates to `False` rather than raising.
- [ ] Membership over a sequence holding non-`Snapshot` elements evaluates
without raising.
- [ ] The `other is None` guard is removed, or what it still does is stated at
the code.
- [ ] Tests and `pre-commit run --all-files` pass.
- [ ] If a caller turns out to depend on the raise, this closes with that
caller named.
## Related issues
#502 fixes the hash side of the same `__eq__` and names this defect in its
refactor commit as deliberately deferred.
Guide de contribution
Ouvrir le guide de contribution
Piste de recherche
Start in zfs/replicate/snapshot/type.py, where Snapshot.__eq__ handles non-Snapshot operands, then inspect zfs_test/replicate_test/snapshot_test/type_test.py for the existing comparison coverage. Update the behavior so comparisons and membership checks with non-Snapshot values do not raise, remove or clarify the None guard, and run the focused tests followed by pre-commit run --all-files.
Rédigé par le modèle d'indexation à partir du texte de l'issue.
Évaluation
- Stack technique
- python
- Domaine
- backend
- Type d'issue
- Bug
- Difficulté
- 2/5
- Temps estimé
- 1-3 heures
- Activité
- Active
- Clarté
- Clairement spécifiée
- Accessibilité débutants
- 88/100