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

fix: No more BytesWarnings #1286

Closed
wants to merge 9 commits into from

Conversation

BYK
Copy link
Contributor

@BYK BYK commented Nov 13, 2024

Fixes #1236.

This patch makes all header operations operate on bytes and converts all headers and values to bytes before operation. With a follow up patch to hpack it should also increase efficiency as currently, hpack casts everything to a str first before converting back to bytes: https://github.com/python-hyper/hpack/blob/02afcab28ca56eb5259904fd414baa89e9f50266/src/hpack/hpack.py#L150-L151

@BYK
Copy link
Contributor Author

BYK commented Nov 13, 2024

Note to reviewer: I'm aware there are formatting changes (they came from "format on save") and although I think things are more readable now, I'm happy to find and revert all unrelated formatting changes to make reviews easier.

Unfortunately, I realized the auto format changes way too late and wanted to get the patch up as promised first.

Alternatively, I can submit a PR that reformats the entire codebase without any changes and then rebase this on top of that.

Fixed format-related things mostly. I think we should still do a full repo black to make it easier for future contributors.

Fixes python-hyper#1236.

This patch makes all header operations operate on `bytes` and converts all headers and values to bytes before operation. With a follow up patch to `hpack` it should also increase efficiency as currently, `hpack` casts everything to a `str` first before converting back to bytes: https://github.com/python-hyper/hpack/blob/02afcab28ca56eb5259904fd414baa89e9f50266/src/hpack/hpack.py#L150-L151
@BYK BYK force-pushed the byk/fix/no-byteswarnings branch from c95e738 to 605f0ea Compare November 14, 2024 10:33
@Kriechi
Copy link
Member

Kriechi commented Nov 14, 2024

Please keep the changes in each PR to a minimum - in this case please revert the style-related changes.
Discussing coding style is outside of the scope here.

@BYK
Copy link
Contributor Author

BYK commented Nov 14, 2024

@Kriechi we should be good now, sorry for the trouble.

src/h2/utilities.py Outdated Show resolved Hide resolved
src/h2/events.py Outdated Show resolved Hide resolved
norm_headers = h2.utilities.normalize_outbound_headers(headers, None, False)
norm_headers = h2.utilities.normalize_outbound_headers(
h2.utilities.utf8_encode_headers(headers), None, False
)
Copy link
Member

Choose a reason for hiding this comment

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

I'm contemplating making the encoding of headers part of the normalization pipeline, instead of being the responsibility of the outside caller.

What do you think about integrating it here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's exactly what I wanted but unfortunately we have an option to bypass normalization, which breaks that neat abstraction :(

Copy link
Member

Choose a reason for hiding this comment

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

utf8_encode_headers is currently called in two places: connections.py - push_stream and stream.py - send_headers. Both share a code path with H2Stream._build_headers_frames.

I'll try to refactor it and share the update to discuss.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds great!

@BYK BYK requested a review from Kriechi November 15, 2024 22:31
@Kriechi Kriechi mentioned this pull request Nov 18, 2024
@Kriechi Kriechi mentioned this pull request Nov 23, 2024
@BYK BYK deleted the byk/fix/no-byteswarnings branch December 18, 2024 22:10
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.

Comparison between bytes and string in defining a frozenset throws exception
2 participants