Conversation
_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. |
sethmlarson
left a comment
sethmlarson
left a comment
There was a problem hiding this comment.
Seems good to me, one question:
|
|
||
|
|
||
| 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?
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.
| _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.
|
Having said I've no attachment to dataclasses; what do you think of the usage of |
|
I'm +1 on using dataclasses and frozen. |
|
See #124 as the dataclass alternative. |
|
I've merged the dataclass approach for the reasons discussed, plus that we have a desire to use dataclasses consistently in the hyper projects. |
3 participants
This is based on #116 so you can ignore the first commit.
_EventBundleuses 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: