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

gh-94909: fix joining of absolute and relative Windows paths in pathlib #95450

Merged
merged 3 commits into from Aug 12, 2022

Conversation

barneygale
Copy link
Contributor

@barneygale barneygale commented Jul 29, 2022

Have pathlib use os.path.join() to join arguments to the PurePath initialiser, which fixes a minor bug when handling relative paths with drives.

Previously:

>>> from pathlib import PureWindowsPath
>>> a = 'C:/a/b'
>>> b = 'C:x/y'
>>> PureWindowsPath(a, b)
PureWindowsPath('C:x/y')

Now:

>>> PureWindowsPath(a, b)
PureWindowsPath('C:/a/b/x/y')

Automerge-Triggered-By: GH:brettcannon

@barneygale
Copy link
Contributor Author

barneygale commented Aug 3, 2022

Hey @serhiy-storchaka, would you be willing to review? You fixed the same issue in joinpath() (#64107) almost a decade ago! Thanks.

Lib/pathlib.py Outdated Show resolved Hide resolved
@brettcannon
Copy link
Member

brettcannon commented Aug 5, 2022

@barneygale if @serhiy-storchaka doesn't review in a week I will go ahead and merge; feel free to ping me to do it if I forget (I'm also stalling because 3.11.0.rc1 is being cut today and I don't want to have to remember to merge into 3.11 once the branch opens again and then forget). 😅

@brettcannon brettcannon linked an issue Aug 5, 2022 that may be closed by this pull request
Co-authored-by: Brett Cannon <brett@python.org>
@serhiy-storchaka
Copy link
Member

serhiy-storchaka commented Aug 6, 2022

Wait a moment. This change goes against the initial approach. It can be slower and inconsistent with other pathlib code (the definition of "drive" was different in pathlib and os.path, and can still be different).

@barneygale
Copy link
Contributor Author

barneygale commented Aug 6, 2022

Could you give an example where its inconsistent with other parts of pathlib?

It might be slower (I haven't checked) but I don't think pathlib, as a high level path library, should care about beating the performance of os.path for equivalent operations. Comparable performance is fine.

@barneygale
Copy link
Contributor Author

barneygale commented Aug 8, 2022

We could de-risk this by skipping backporting. It's a niche bug, and I only need it solved in main.

@brettcannon brettcannon added 🤖 automerge and removed needs backport to 3.10 needs backport to 3.11 labels Aug 12, 2022
@miss-islington miss-islington merged commit 187949e into python:main Aug 12, 2022
14 checks passed
@brettcannon
Copy link
Member

brettcannon commented Aug 12, 2022

To keep things simple I only did this to main. We can always backport later if we find it necessary.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🤖 automerge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants