Get rid of _EventBundle - #118
Conversation
Sorry, something went wrong.
_EventBundle uses a lot of dynamic python features to save on some duplication, but it slows things down, and will also make it much harder to add static typing. Since these types are now pretty stable, it seems not worth it. On the bench/ micro-benchmark: Before: 9322.6 requests/sec After : 10544.6 requests/sec
|
I'm happy with this, I've no strong attachment to dataclasses. I'm minded to merge after #116. |
Sorry, something went wrong.
sethmlarson
left a comment
There was a problem hiding this comment.
Seems good to me, one question:
Sorry, something went wrong.
|
|
||
|
|
||
| class ConnectionState(object): | ||
| class ConnectionState: |
There was a problem hiding this comment.
Should we use __slots__ for this object as well since it's accessed frequently?
Sorry, something went wrong.
There was a problem hiding this comment.
I'm in favor of adding slots to everything personally. I'll do it in a separate PR after some of the others are settled.
Sorry, something went wrong.
| _defaults = {"http_version": b"1.1", "reason": b""} | ||
| # Useful for tests | ||
| def __eq__(self, other): | ||
| if not isinstance(other, type(self)): |
There was a problem hiding this comment.
Whilst this changes the logic of what is equal or not (from self.__class__ == other.__class__) to include subclasses and potentially raising if incompatible classes are compared (rather than returning False) I think it is fine as this is mostly (and likely only) used for tests.
Sorry, something went wrong.
|
Having said I've no attachment to dataclasses; what do you think of the usage of frozen as in wsproto? I think that better clarifies the API for these objects. |
Sorry, something went wrong.
|
I'm +1 on using dataclasses and frozen. |
Sorry, something went wrong.
|
I've merged the dataclass approach for the reasons discussed, plus that we have a desire to use dataclasses consistently in the hyper projects. |
Sorry, something went wrong.
This is based on #116 so you can ignore the first commit.
_EventBundle uses a lot of dynamic python features to save on some duplication, but it slows things down, and will also make it much harder to add static typing. Since these types are now pretty stable, it seems not worth it.
On the bench/ micro-benchmark: