alunduil / alunduil/zfs-replicate

Snapshot equality returns NotImplemented for a non-Snapshot operand

Ouverte Adaptée aux débutants
#694 0 commentaires 0 réactions 0 personnes assignées Voir sur GitHub
bug
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

Recevez les nouvelles issues par e-mail

Un résumé court des issues GitHub adaptées aux débutants.