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 to email header parser from deprecated cgi library #195

Merged
merged 2 commits into from
Oct 28, 2024

Conversation

pib
Copy link
Contributor

@pib pib commented Oct 18, 2024

Fixes #160

Based on the answer here https://stackoverflow.com/a/77225775

Also removed the use of cgi.parse_header for the location header since as far as I can tell, that was never actually necessary.

@peterbe
Copy link
Owner

peterbe commented Oct 28, 2024

Hmm. Why aren't tests running??

@hartwork
Copy link
Collaborator

@peterbe it says "2 workflows awaiting approval". You need to press the "Approve" button.

@peterbe
Copy link
Owner

peterbe commented Oct 28, 2024

I think it's a bug. That option isn't appearing.

Mergebox

@hartwork
Copy link
Collaborator

I think it's a bug. That option isn't appearing.

@peterbe could be!

What I see:

what_i_see

@peterbe
Copy link
Owner

peterbe commented Oct 28, 2024

When I switch to the old merge box experience, that button appears.
OLd mergebox

@hartwork
Copy link
Collaborator

When I switch to the old merge box experience

@peterbe could you elaborate what you mean by that and what you did to change now?

hashin.py Outdated Show resolved Hide resolved
Co-authored-by: Sebastian Pipping <[email protected]>
@peterbe
Copy link
Owner

peterbe commented Oct 28, 2024

When I switch to the old merge box experience

@peterbe could you elaborate what you mean by that and what you did to change now?

Perhaps because I'm GitHub staff, I have access to a preview version of the merge-box. So mine looks different.
I disabled the preview version and went back to the regular one, and then that option appears.

@peterbe peterbe requested a review from hartwork October 28, 2024 13:57
Copy link
Owner

@peterbe peterbe left a comment

Choose a reason for hiding this comment

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

if tests are passing, I'm cool with it.

@peterbe
Copy link
Owner

peterbe commented Oct 28, 2024

if tests are passing, I'm cool with it.

You, @hartwork ?

@hartwork
Copy link
Collaborator

if tests are passing, I'm cool with it.

You, @hartwork ?

@peterbe I'm looking at it now from a few different angles, I'm hoping to come back with a result within the next hour.

Copy link
Collaborator

@hartwork hartwork left a comment

Choose a reason for hiding this comment

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

@peterbe I have done two things now for review:

  1. Check what the HTTP 1.1 spec had to say on the value of header Location back then at https://datatracker.ietf.org/doc/html/rfc2616#section-14.30. Good news is it's just a URI so the change here is fine. Bad news is: It no longer has to be absolute, I'll open a dedicated issue in a minute about how that breaks hashin code (but it is unrelated to the issue at hand here).

  2. Play with the new code with a non-trivial example of a Content-type value, looks good, see below:

In [1]: content_type = 'application/json; charset="utf8"'

In [2]: import cgi

In [3]: cgi.parse_header(content_type)
Out[3]: ('application/json', {'charset': 'utf8'})

In [4]: from email.headerregistry import HeaderRegistry

In [5]: parsed = HeaderRegistry()("content-type", content_type)

In [6]: parsed.content_type
Out[6]: 'application/json'

In [7]: parsed.params['charset']
Out[7]: 'utf8'

So approving… 👍

Thanks @pib for taking us into the Python 3.13 era with hashin! 🙏

@peterbe peterbe merged commit 7a24881 into peterbe:master Oct 28, 2024
5 checks passed
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.

Python 3.13 support (i.e. need to migrate off of stdlib package cgi)
3 participants