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

Make h2.events more typing-friendly #1296

Open
wants to merge 14 commits into
base: master
Choose a base branch
from
Open

Conversation

mhils
Copy link
Member

@mhils mhils commented Feb 4, 2025

This PR adds proper constructors for most of the h2 events. This allows us to tighten the type annotations, simplifying type checks in downstream implementations. These events should not be instantiated by anyone except h2 itself, so it shouldn't be a breaking change.

Based on #1297.

@mhils mhils requested a review from Kriechi February 4, 2025 00:11
@mhils mhils force-pushed the ctors branch 2 times, most recently from 13361d9 to ab9c1c6 Compare February 4, 2025 01:01
@Kriechi
Copy link
Member

Kriechi commented Feb 4, 2025

LGTM! Thanks!

These events should not be instantiated by anyone except h2 itself, so it shouldn't be a breaking change.

I assume you intended this as "most users will not use this API and won't be affected by this change"?
And not in the way of "this API should be made private and documented as do-not-use"?

Please add a quick note into the changelog. Thanks!

@mhils
Copy link
Member Author

mhils commented Feb 4, 2025

I assume you intended this as "most users will not use this API and won't be affected by this change"?

If by "this API" you mean the __init__ method, then yes.

I will add constructors for the other events and then also a changelog entry.

@mhils
Copy link
Member Author

mhils commented Feb 5, 2025

I've switched to @dataclass to make things simpler and less repetitive. What do you think?

Python 3.9 requires some extra logic because it doesn't have kw_only yet. We can drop kw_only entirely if you'd like, even though I think it's generally a good idea.

@Kriechi
Copy link
Member

Kriechi commented Feb 7, 2025

I do like dataclasses!

I do like the kw_only pattern!

I don't like the extra fuzz about py3.9. But since py3.9 becomes EOL later this year, I think its acceptable. Happy to hear other thoughts on it!

@mhils mhils changed the title Add typing-friendly constructor for events.WindowUpdated Make h2.events more typing-friendly Feb 7, 2025
@mhils
Copy link
Member Author

mhils commented Feb 7, 2025

This is ready for review now. With these changes we get from 31 mypy complaints down to 3 in mitmproxy (see linked PR for the fixes). All other downstream consumers should profit similarly.

src/h2/events.py Outdated Show resolved Hide resolved
Copy link
Member

@Kriechi Kriechi left a comment

Choose a reason for hiding this comment

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

Please add a changelog entry.

The docs should be auto-generated, or do you think we need to update something?
https://github.com/python-hyper/h2/blob/master/docs/source/api.rst#events

LGTM! Thanks!

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