-
-
Notifications
You must be signed in to change notification settings - Fork 31.8k
GH-125866: Support complete "file:" URLs in urllib #132378
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
Conversation
Add optional *add_scheme* argument to `urllib.request.pathname2url()`; when set to true, a complete URL is returned. Likewise add optional *has_scheme* argument to `urllib.request.url2pathname()`; when set to true, a complete URL is accepted.
Some Qs for reviewers:
Thanks in advance :] |
If I were to chose one, I would use
I don't think we need to wait if we can ship them. Waiting means that users also wait for at least 2y until they can consider 3.15 as the stable release (and I guess many like having the latest stable release as their main version). |
Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>
Thank you for the review + feedback - much appreciated :) I'm not sure how I feel about |
A bit, but at least users don't need to remember different names for the path->url and url->path functions. |
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.
LGTM. 👍
Well, they didn't have to until we made the argument keyword-only 🙈 |
True! but on the other hand, if I see |
I'm going to ask on discord, as I like |
+1 for keyword-only (non-keyword boolean flags read poorly when called with boolean literals, and reducing the API surface by restricting the calling style generally makes future maintenance easier) For the parameter naming though, is the flag on I also think the ambiguity concern with a unified flag name is valid, so I'd be inclined to keep |
The prefix to remove would be >>> pathname2url('hello')
'hello'
>>> url2pathname('foo:bar')
'foo:bar'
>>> url2pathname('file:bar')
'file:bar' Though it's still very unlikely, I worry that |
It was a pain to handle URLs like "file:123" where "file" was interpreted as a host name and "123" as a port number.. |
That makes sense - the input ambiguity would be much worse than I initially thought it would be. I'd suggest |
I hadn't thought about the expected contract and the parameter name so |
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.
Just making sure that we have kw-only at runtime as well and LGTM.
I like require_scheme too! I've adjusted the PR to use that. One last thought: we could maybe accept a >>> url = 'file:///C:/Program%20Files'
>>> url2pathname(url, scheme='file')
'C:\\Program Files'
>>> path = 'C:\\Program Files'
>>> pathname2url(path, scheme='file')
'file:///C:/Program%20Files' ... but I can't really think of a use case for values other than |
Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>
I also don't know of another scheme for local paths and for which |
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.
Some nits but you can go ahead whenever you want.
Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>
Add optional add_scheme argument to
urllib.request.pathname2url()
; when set to true, a complete URL is returned. Likewise add optional require_scheme argument tourl2pathname()
; when set to true, a complete URL is accepted.urllib.request
#125866📚 Documentation preview 📚: https://cpython-previews--132378.org.readthedocs.build/