alunduil / alunduil/zfs-replicate

Snapshot equality returns NotImplemented for a non-Snapshot operand

オープン 初心者向け
#694 コメント 0 件 リアクション 0 件 担当者 0 名 GitHub で見る
bug
主要言語
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

新しい issue をメールで受け取る

初心者向けの GitHub issue を短くまとめたダイジェスト。