Improve path_to_url() tests
#13496
Improve path_to_url() tests
#13496
Conversation
barneygale
force-pushed
the
fix-pathname2url-tests
branch
from
9572b92 to
19cdc19
Compare
Use `urllib.parse.quote()` rather than `urllib.request.pathname2url()` to generate expected URLs in tests for `path_to_url()`. This makes expectations a little more explicit. (The implementation of `path_to_url()` itself calls `pathname2url()`, and so we were marking our own homework!)
barneygale
force-pushed
the
fix-pathname2url-tests
branch
from
e75c6a0 to
7caaf5d
Compare
| @@ -11,15 +11,15 @@ | |||
| def test_path_to_url_unix() -> None: | |||
| assert path_to_url("/tmp/file") == "file:///tmp/file" | |||
| path = os.path.join(os.getcwd(), "file") | |||
| assert path_to_url("file") == "file://" + path | |||
| assert path_to_url("file") == "file://" + urllib.parse.quote(path) | |||
There was a problem hiding this comment.
Without this change, tests will fail if the working directory contains a non-urlsafe character.
|
Unfortunately as I've already mentioned, I am utterly useless when it comes to URLs and filenames. I'd prefer if someone with a bit more expertise reviewed this. Alternatively, I can review this, but it will be pretty far down on my priority list as I have a release to cut in a few days. |
|
Ah OK, no worries. I'll wait for someone else to review I suppose. |
| path = os.path.join(os.getcwd(), "file").replace("\\", "/") | ||
| assert path_to_url("file") == "file:///" + urllib.parse.quote(path, safe="/:") |
There was a problem hiding this comment.
Just to be sure, file:c:/file (and file:/c:/file) is the non-normative form of file:///c:/file but they are effectively equivalent, right?
There was a problem hiding this comment.
Also, on Windows, does pathname2url() also include : in its url-safe default list? I'm on Linux, and pathname2url("C:\\Program Files") returns file:C%3A%5CProgram%20Files which is fine for Linux, but would be nonsensial for Windows.
There was a problem hiding this comment.
Just to be sure,
file:c:/file(andfile:/c:/file) is the non-normative form offile:///c:/filebut they are effectively equivalent, right?
That's correct.
Also, on Windows, does
pathname2url()also include:in its url-safe default list?
It does for the drive, yes: https://github.com/python/cpython/blob/6784ef7da7cbf1a944fd0685630ced54e4a0066c/Lib/urllib/request.py#L1711
(NTFS allows colons in other parts of the path, but Windows does not.)
There was a problem hiding this comment.
I did some more reading on the URL RFCs. This looks good. I'll merge this now to make progress towards unblocking the release, but more work will be needed to fully fix 3.14 CI.
In addition, a post-merge review from people with more expertise than me (when time permits) would be appreciated.
|
Also thank you @barneygale! I will need your help with resolving the remaining failures in #13506. |
Use
urllib.parse.quote()rather thanurllib.request.pathname2url()to generate expected URLs in tests forpath_to_url(). This makes expectations a little more explicit.(The implementation of
path_to_url()itself callspathname2url(), and so we were marking our own homework!)Also stop relying on
urllibto transform Windows drive letters to uppercase (this behaviour was removed in 3.14.)