-
Notifications
You must be signed in to change notification settings - Fork 10
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
Properly parse the content disposition filename #46
Conversation
Add a test please? |
What kind of test would you like? There's one static lib package in my fork that triggers the problem, but of course their server could stop wrapping the filename in quotes altogether. We could also extract the header parsing into a helper function and unit test it - if so I could also expected failure tests for other cases of the header that aren't handled yet (until someone else needs them). Alternatively, is there some library that could do the header parsing for us so we don't need these half-baked fixes? |
@hoodmane We're now using the |
f29a8fc
to
3e3ffe4
Compare
It would still be good to have a test, particularly because it sounds like it will be quite simple. If there is some reason that adding a test would be very hard it can be omitted but that doesn't seem to me to be the case here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
Just have to fix pre-commit... |
Use the email.message.Message parser Refactor into _extract_tarballname helper function Add a small unit test
4503b4a
to
cb850bc
Compare
@hoodmane This should now be good to go :) |
Thanks @juntyr! |
If the content disposition header filename uses quotes,
pyodide-build
includes the quotes in the literal filename, which e.g. confusesshutil.unpack_archive
in determining the file type.This PR
includes a simple fix to check and strip the quotesuses theemail.message.Message
parser to properly extract the filename (see https://stackoverflow.com/a/78073510).The PR should be squashed (the first commit is overriden by the second one).