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

Allow urlparse and urlsplit of bytes URLs #9746

Merged
merged 4 commits into from
Feb 17, 2023

Conversation

koogoro
Copy link
Contributor

@koogoro koogoro commented Feb 17, 2023

Right now, urlparse(url) and urlsplit(url) doesn't work if url is a bytes. This is because the only overload that can match this case is the one where url: bytes | bytearray, but even this one doesn't match since the default value for scheme is "", which isn't bytes | bytearray | None. Adding Literal[""] to the types for scheme won't cause the overloads to overlap, since they're differentiated by the type of url, and will allow urlparse applied to a single bytes argument to work again.

Copy link
Collaborator

@hauntsaninja hauntsaninja left a comment

Choose a reason for hiding this comment

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

Thanks, this matches the runtime better!

@github-actions

This comment has been minimized.

Copy link
Member

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

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

Thanks! Looks like we could actually combine the first two overloads of urlparse and urlsplit in that case — want to do that?

@koogoro
Copy link
Contributor Author

koogoro commented Feb 17, 2023

Is there an easy way to do that? It seems hard to me since they still have different return types, but if there's a good way to combine them then I'll do it.

@AlexWaygood
Copy link
Member

AlexWaygood commented Feb 17, 2023

Is there an easy way to do that? It seems hard to me since they still have different return types, but if there's a good way to combine them then I'll do it.

Good point — we can't combine the overloads for urlparse, but we can combine three overloads urlsplit, where the return types are the same for overloads two and three

@AlexWaygood
Copy link
Member

Sorry, I meant to combine the second and third overloads, not the first two overloads! Apologies for the garbled review

Copy link
Member

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

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

Thanks!

@github-actions
Copy link
Contributor

According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉

@AlexWaygood AlexWaygood merged commit 5ebf892 into python:main Feb 17, 2023
hauntsaninja pushed a commit to python/mypy that referenced this pull request Feb 18, 2023
Syncing typeshed again to make sure
python/typeshed#9746 gets into the 1.1 release.
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.

None yet

3 participants