-
-
Notifications
You must be signed in to change notification settings - Fork 33.5k
bpo-41357: Add a sentence to os.path.abspath() clarifying that it pre… #21596
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
…serves symlink names. Also, replace "os.path.abspath" with "os.path.realpath" in the table at the end of the Path class section.
Doc/library/os.path.rst
Outdated
| platforms, this is equivalent to calling the function :func:`normpath` as | ||
| follows: ``normpath(join(os.getcwd(), path))``. | ||
| follows: ``normpath(join(os.getcwd(), path))``. Unlike :func:`realpath`, | ||
| this function preserves symlink names. |
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.
I think this should be phrased in the negative that "this function does not resolve symlink targets".
If the reader follows through to the documentation of normpath, it's clear that it resolves ".." components, which may remove symlinks from the path. But based on just this sentence, it's not clear what it means to preserve symlinks. A different implementation might retain all ".." components, as pathlib's absolute method does. But the current implementation can change the real target of a Unix path, e.g. normpath('spam/symlink/../eggs') -> "spam/eggs".
In Windows, this is usually not a problem since the Windows API works like this anyway. The one exception is in the target of a relative symlink, e.g. symlink(r'spam\symlink\..\eggs', 'eggs') is not the same as symlink(r'spam\eggs', 'eggs').
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.
I updated the wording to more closely reflect, in a negative fashion, the language used for realpath().
eamanu
left a comment
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.
I don't know if is necessary a NEWS here.
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.
Thanks, looks like the pathlib change is on main. If you want to add the extra line to os.path.abspath, please open a new PR :-) (and yes, news entries aren't needed for docs changes)
|
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 |
…serves symlink names. Also, replace "os.path.abspath" with "os.path.realpath" in the table at the end of the Path class section.
https://bugs.python.org/issue41357