Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Switch event classes to dataclasses #124

Merged
merged 1 commit into from
May 16, 2021
Merged

Conversation

pgjones
Copy link
Member

@pgjones pgjones commented Dec 27, 2020

The _EventBundle used previously dynamically creates the Event
classes, which makes typing hard. As the Event classes are now stable
the dynamic creation isn't required, and as h11 supports Python3.6+
dataclasses can be used instead.

This now makes the Events frozen (and almost imutable) which better
matches the intended API. In turn it requires the object.__setattr__
usage and the alteration to the
_clean_up_response_headers_for_sending method.

This change also improves the performance a little, using the
benchmark,

Before: 6.7k requests/sec
After: 6.9k requests/sec

Notes:

The test response-header changes are required as the previous version
would mutate the response object.

The init for the Data event is required as slots and defaults aren't
possible with dataclasses.

Copy link
Contributor

@bluetech bluetech left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am personally not a big fun of using dataclasses or attrs in a public library API, because

  1. They add some startup overhead (probably negligible in this case though).
  2. They add "hidden" APIs to the classes, like dataclasses.asdict, dataclasses.replace etc. which users will eventually rely on, making it part of the public API.
  3. They somewhat reduce (or add friction to) the library's control should it ever need it.
  4. They tend to do some funky stuff, for example in pytest we had some trouble with what dataclasses does here.

That said, if you're OK with this, the code LGTM except one comment.

h11/_events.py Show resolved Hide resolved
The _EventBundle used previously dynamically creates the Event
classes, which makes typing hard. As the Event classes are now stable
the dynamic creation isn't required, and as h11 supports Python3.6+
dataclasses can be used instead.

This now makes the Events frozen (and almost imutable) which better
matches the intended API. In turn it requires the `object.__setattr__`
usage and the alteration to the
`_clean_up_response_headers_for_sending` method.

This change also improves the performance a little, using the
benchmark,

Before: 6.7k requests/sec
After: 6.9k requests/sec

Notes:

The test response-header changes are required as the previous version
would mutate the response object.

The init for the Data event is required as slots and defaults aren't
possible with dataclasses.
@pgjones pgjones merged commit 5d958b5 into python-hyper:master May 16, 2021
@pgjones pgjones deleted the dc branch May 16, 2021 09:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants