-
-
Notifications
You must be signed in to change notification settings - Fork 30.7k
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
bpo-29688: document and test pathlib.Path.absolute()
.
#26153
bpo-29688: document and test pathlib.Path.absolute()
.
#26153
Conversation
Documentation is clear and the suggested changes looks sane to me. Will take another look after windows test failures are addressed. |
Tested are fixed, thanks for taking a look. |
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.
Some small remarks.
Note that this can't be automatically backported, as the tests' patching of |
This PR is stale because it has been open for 30 days with no activity. |
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.
One clarifying doc change as suggested by @domdfcoding. A news entry is also missing.
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 |
Sorry for the long delay. Does this need a NEWS entry? It adds docs and tests but doesn't change the implementation of |
Co-authored-by: Brett Cannon <brett@python.org>
Co-authored-by: Brian Helba <brian.helba@kitware.com>
644703a
to
64204fd
Compare
It looks like you added a news entry anyway. 😄 |
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.
Question about the docs and some very minor formatting things.
Misc/NEWS.d/next/Library/2022-01-05-03-21-21.bpo-29688.W06bSH.rst
Outdated
Show resolved
Hide resolved
Co-authored-by: Brett Cannon <brett@python.org>
self.assertEqual(str(P('c:\\').absolute()), 'c:\\') | ||
self.assertEqual(str(P('c:\\a').absolute()), 'c:\\a') | ||
self.assertEqual(str(P('c:\\a\\b').absolute()), 'c:\\a\\b') | ||
|
||
# UNC absolute paths. | ||
share = '\\\\server\\share\\' | ||
self.assertEqual(str(P(share).absolute()), share) | ||
self.assertEqual(str(P(share + 'a').absolute()), share + 'a') | ||
self.assertEqual(str(P(share + 'a\\b').absolute()), share + 'a\\b') |
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.
self.assertEqual(str(P('c:\\').absolute()), 'c:\\') | |
self.assertEqual(str(P('c:\\a').absolute()), 'c:\\a') | |
self.assertEqual(str(P('c:\\a\\b').absolute()), 'c:\\a\\b') | |
# UNC absolute paths. | |
share = '\\\\server\\share\\' | |
self.assertEqual(str(P(share).absolute()), share) | |
self.assertEqual(str(P(share + 'a').absolute()), share + 'a') | |
self.assertEqual(str(P(share + 'a\\b').absolute()), share + 'a\\b') | |
self.assertEqual(str(P(r'c:\').absolute()), r'c:\') | |
self.assertEqual(str(P(r'c:\a').absolute()), r'c:\a') | |
self.assertEqual(str(P(r'c:\a\b').absolute()), r'c:\a\b') | |
# UNC absolute paths. | |
share = '\\\\server\\share\\' | |
self.assertEqual(str(P(share).absolute()), share) | |
self.assertEqual(str(P(share + 'a').absolute()), share + 'a') | |
self.assertEqual(str(P(share + r'a\b').absolute()), share + r'a\b') |
Thanks, @barneygale ! |
Thanks very much for the review! |
This method is important and cannot be replaced with
resolve()
!resolve()
will hit OS APIs to resolve symlinks/etc, and will fail on symlink loops, whereasabsolute()
just prepends the working directory and can never raise an OSError.Also improved the table showing correspondence with
os.path
methods. The gory details:Path.resolve()
isos.path.realpath()
in strict mode, with a tweaked exception type for symlink loops (backwards compat)Path.absolute()
isos.path.abspath()
but without normalizing the path. The pathlib behaviour is better, asabspath()
's normlisation changes the meaning of the path in the presence of symlinks!Happy to expand the tests if anyone has any ideas what else to cover.
https://bugs.python.org/issue29688