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

URL refactoring #206

Merged
merged 1 commit into from
Apr 13, 2024
Merged

URL refactoring #206

merged 1 commit into from
Apr 13, 2024

Conversation

ml31415
Copy link
Collaborator

@ml31415 ml31415 commented Apr 12, 2024

The current URL implementation did quite some parsing, which is nowadays already done in a more robust and clean way in the standard library. This PR preserves the previous URL interface, while leaving all the actual work to urllib.parse.urlparse and urllib.parse.ParseResult.

It would be debatable, if we shouldn't drop the url module entirely and just use urllib.parse and all tools within directly without any wrapping. Mb something to keep in mind for a potential future major version bump.

@cyberw
Copy link
Collaborator

cyberw commented Apr 13, 2024

I guess what we lose from this change is the slots-optimization? Dont know if it matters…

@ml31415
Copy link
Collaborator Author

ml31415 commented Apr 13, 2024

Not really. The ParsedResult is a namedtuple, i.e. "slots". The benchmarks are not impacted anyways, as these URL operations, very unlike the header parsing, are not a bottleneck.

I had initially tried to directly derive from ParsedResult, avoiding this indirection, but that fails, as I'd have had to change the constructor syntax, like Url.from_string(...) or smth. Or def URL(...)as constructor and ParsedURL() as result. Anyways, all of it would introduce breaking changes.

@cyberw
Copy link
Collaborator

cyberw commented Apr 13, 2024

Lgtm

@ml31415 ml31415 merged commit d673ea9 into master Apr 13, 2024
12 checks passed
@ml31415 ml31415 deleted the url_cleanup branch April 15, 2024 11:26
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

2 participants