reset the __eq__ and __hash__ of HTTPConnection to allow WebSockets to be added to …#1039
reset the __eq__ and __hash__ of HTTPConnection to allow WebSockets to be added to …#1039graingert merged 3 commits intoKludex:masterfrom
__eq__ and __hash__ of HTTPConnection to allow WebSockets to be added to …#1039Conversation
__eq__ and __hash__ to allow WebSockets to be added to …__eq__ and __hash__ of HTTPConnection to allow WebSockets to be added to …
|
@JayH5 is this a good change? |
|
Could you explain the motivation a bit more clearly here? |
|
@tomchristie for a number of reasons it's really useful to be able to put connections in a set. Especially websocket pubsub, see fastapi/fastapi#1991 I can see no use for comparing non-identical connections for equality |
| return len(self.scope) | ||
|
|
||
| __eq__ = object.__eq__ | ||
| __hash__ = object.__hash__ |
There was a problem hiding this comment.
Doesn't the hash method on here already inherit object.__hash__? What am I missing?
There was a problem hiding this comment.
Mapping overrides __eq__ which disables the default __hash__
There was a problem hiding this comment.
Mapping overrides eq
Ah gotcha, thanks.
Does that mean we actually only need to override __eq__ then?
What do we think to something like the following?...
# Don't use the `abc.Mapping.__eq__` implementation.
# Connection instances should never be considered equal
# unless `self is other`.
__eq__ = object.__eq__There was a problem hiding this comment.
I think you need to re-enable __hash__ but I'll double check
There was a problem hiding this comment.
@tomchristie yep you need both:
from __future__ import annotations
import collections.abc
import pytest
def test_default():
class HTTPConnection(collections.abc.Mapping):
def __getitem__(self, *args, **kwargs):
return None
def __iter__(self, *args, **kwargs):
return self
def __next__(self, *args, **kwargs):
raise StopIteration
def __len__(self, *args, **kwargs):
return 0
assert HTTPConnection() == HTTPConnection()
with pytest.raises(TypeError, match=r"unhashable type: 'HTTPConnection'"):
connections = {HTTPConnection()}
def test_eq():
class HTTPConnection(collections.abc.Mapping):
__eq__ = object.__eq__
def __getitem__(self, *args, **kwargs):
return None
def __iter__(self, *args, **kwargs):
return self
def __next__(self, *args, **kwargs):
raise StopIteration
def __len__(self, *args, **kwargs):
return 0
assert HTTPConnection() != HTTPConnection()
h = HTTPConnection()
assert h == h
with pytest.raises(TypeError, match=r"unhashable type: 'HTTPConnection'"):
connections = {HTTPConnection()}
def test_eq_and_hash():
class HTTPConnection(collections.abc.Mapping):
__eq__ = object.__eq__
__hash__ = object.__hash__
def __getitem__(self, *args, **kwargs):
return None
def __iter__(self, *args, **kwargs):
return self
def __next__(self, *args, **kwargs):
raise StopIteration
def __len__(self, *args, **kwargs):
return 0
assert HTTPConnection() != HTTPConnection()
h = HTTPConnection()
assert h == h
assert {HTTPConnection()} != {HTTPConnection()}
assert h in {h}
assert {h} == {h}
lovelydinosaur
left a comment
There was a problem hiding this comment.
Okay, fair enough. 😄
| ) | ||
| assert websocket == websocket | ||
| assert websocket in {websocket} | ||
| assert {websocket} == {websocket} |
There was a problem hiding this comment.
Yup, that's all neat enough. 👍
…sets
https://github.com/tiangolo/fastapi/pull/1991/files#diff-a3e1b3437a67897cf7539e9ec348c9d8R50