Skip to content

Conversation

@websurfer5
Copy link
Contributor

@websurfer5 websurfer5 commented Jul 23, 2020

…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

…serves symlink names. Also, replace "os.path.abspath" with "os.path.realpath" in the table at the end of the Path class section.
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.
Copy link
Contributor

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').

Copy link
Contributor Author

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().

Copy link
Contributor

@eamanu eamanu left a 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.

Copy link
Contributor

@hauntsaninja hauntsaninja left a 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)

@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 phrase I 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.

@hauntsaninja hauntsaninja added pending The issue will be closed if no feedback is provided and removed pending The issue will be closed if no feedback is provided labels Jan 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

awaiting changes docs Documentation in the Doc dir

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants