Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

gh-99029: Fix handling ofPureWindowsPath('C:\<blah>').relative_to('C:')#99031

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
brettcannon merged 5 commits intopython:mainfrombarneygale:pathlib-relpath
Nov 25, 2022

Conversation

barneygale
Copy link
Contributor

@barneygalebarneygale commentedNov 2, 2022
edited
Loading

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.

…e_to('C:')``relative_to()` now treats naked drive paths as relative. This brings itsbehaviour in line with other parts of pathlib, and with `ntpath.relpath()`,and so allows us to factor out the pathlib-specific implementation.
@domragusa
Copy link
Contributor

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
Copy link
ContributorAuthor

I didn't remove any tests. Please share an example of where this breaks.

@barneygale
Copy link
ContributorAuthor

I told a lie - I actually did remove two assertions that supply strings to the method. Will fix!

@domragusa
Copy link
Contributor

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 callGetFullPathNameW, 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.
Can you test it with names and characters that are reserved on Windows but are allowed on Posix-systems?

I would have just tweaked the fail condition to make sure that we raise the exception for the bug you noticed.

@barneygale
Copy link
ContributorAuthor

barneygale commentedNov 3, 2022
edited
Loading

👍 thanks, I'll try that out!

It's true thatrelpath() can callos.getcwd(). However, the working directory only contributes to the resultif we pass one relative path and one absolute. And that's specifically disallowed in pathlib'srelative_to(). In other cases, the two matching prefixes cancel eachother out incommonpath().

The fact thatrelpath() needlessly callsabspath() when given two relative paths is regrettable, and probably worth fixing. But it shouldn't have an observable effect here. I'll try to find a counterexample.

@domragusa
Copy link
Contributor

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.

@barneygale
Copy link
ContributorAuthor

barneygale commentedNov 3, 2022
edited
Loading

I don't think that matters -os.getcwd() can return something totally nonsensical and we wouldn't care, as long as it returns the same thing twice in a row, so that the shared prefix can be eliminated. Right?

@domragusa
Copy link
Contributor

You're referencingos.getcwd() because you're looking at the implementation ofos.path.abspath inos.posixpath but on Windowsos.path resolves toos.ntpath and in that moduleos.path.abspath calls_getfullpathname() that calls the windows api.

@barneygale
Copy link
ContributorAuthor

Again, I don't think it matters. If you think it does, could you please provide a reproduction case. Thank you.

@barneygale
Copy link
ContributorAuthor

I've logged#99199 to coverrelpath() needlessly callingabspath() when the paths' anchors match.

Differences in handling of '..' parts still make these functionsfundamentally incompatible.
@barneygale
Copy link
ContributorAuthor

Turns out there's another incompatibility related to leading../ segments - see#99199 (comment)

I've revised the implementation to not userelpath() :)

Copy link
Member

@brettcannonbrettcannon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I believe this is in line with the result of the discussion in the issue, so only last change is to use f-strings!

@bedevere-bot
Copy link

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 phraseI have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

@barneygale
Copy link
ContributorAuthor

I have made the requested changes; please review again.

@bedevere-bot
Copy link

Thanks for making the requested changes!

@brettcannon: please review the changes made to this pull request.

@brettcannon
Copy link
Member

Thanks!

@barneygale
Copy link
ContributorAuthor

Thank you!

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@brettcannonbrettcannonbrettcannon approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

4 participants
@barneygale@domragusa@bedevere-bot@brettcannon

[8]ページ先頭

©2009-2025 Movatter.jp