alunduil / alunduil/zfs-replicate
Snapshot equality returns NotImplemented for a non-Snapshot operand
- 主要言語
- Python
- スター
- 24
- フォーク
- 6
- 平均マージ
- 3時間 11分
- マージ済み PR(30日)
- 49
説明
## 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.
コントリビューションガイド
調査の方向性
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.
索引モデルが issue の本文から書いたものです。
評価
- 技術スタック
- python
- 領域
- backend
- issue の種類
- バグ
- 難易度
- 2/5
- 見積もり時間
- 1〜3時間
- 活発さ
- 活発
- 明瞭さ
- 明確に書かれている
- 初心者へのやさしさ
- 88/100