Uh oh!
There was an error while loading. Please reload this page.
gh-99029: Fix handling of PureWindowsPath('C:\<blah>').relative_to('C:') - #99031
Conversation
…e_to('C:')`
`relative_to()` now treats naked drive paths as relative. This brings its
behaviour in line with other parts of pathlib, and with `ntpath.relpath()`,
and so allows us to factor out the pathlib-specific implementation.domragusa
commented
Nov 3, 2022
The implementation of os.path.relpath accesses the filesystem making the two paths absolute and then calculating the common path, using that function to implement a PurePath method is wrong becase it should work without the any file or directory being there (its definition is "Base class for manipulating paths without I/O"), the use case that it breaks is when we don't have files on disk for example I use it to create the structure of a zip archive. Removing tests is not a good idea. Mixing relative and absolute paths should raise an exception as you stated and should be fixed. |
barneygale
commented
Nov 3, 2022
I didn't remove any tests. Please share an example of where this breaks. |
barneygale
commented
Nov 3, 2022
I told a lie - I actually did remove two assertions that supply strings to the method. Will fix! |
domragusa
commented
Nov 3, 2022
I thought that os.path.relpath accessed the filesystem, but taking a better look at it I guess it doesn't. I don't have a Windows machine and I can't provide a concrete example, I'm sorry, anyway it calls os.path.abspath and following the chain we arrive at a Windows api call GetFullPathNameW, that's where I expect PurePath to break because it should be platform independent and this change would make it depend on the result of a winapi call. I would have just tweaked the fail condition to make sure that we raise the exception for the bug you noticed. |
👍 thanks, I'll try that out! It's true that The fact that |
domragusa
commented
Nov 3, 2022
It's not just getcwd, for example "c:" is a valid filename on Linux: on Windows after abspath you would get the current directory on the C drive while on Linux it's the file named "c:" in the current working directory so the calculations that follow would be wrong, there are also reserved names on Windows like COM or CON and characters that are not allowed. |
I don't think that matters - |
domragusa
commented
Nov 3, 2022
You're referencing |
barneygale
commented
Nov 3, 2022
Again, I don't think it matters. If you think it does, could you please provide a reproduction case. Thank you. |
barneygale
commented
Nov 7, 2022
I've logged #99199 to cover |
Differences in handling of '..' parts still make these functions fundamentally incompatible.
barneygale
commented
Nov 12, 2022
Turns out there's another incompatibility related to leading I've revised the implementation to not use |
brettcannon
left a comment
There was a problem hiding this comment.
I believe this is in line with the result of the discussion in the issue, so only last change is to use f-strings!
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
bedevere-bot
commented
Nov 18, 2022
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
barneygale
commented
Nov 19, 2022
I have made the requested changes; please review again. |
bedevere-bot
commented
Nov 19, 2022
Thanks for making the requested changes! @brettcannon: please review the changes made to this pull request. |
brettcannon
commented
Nov 25, 2022
Thanks! |
barneygale
commented
Nov 25, 2022
Thank you! |
relative_to()now treats naked drive paths as relative. This brings its behaviour in line with other parts of pathlib,and withntpath.relpath(), and so allows us to factor out the pathlib-specific implementation.PureWindowsPath.relative_to()andntpath.relpath()with rootless drives #99029