bpo-29688: Document Path.absolute #384
Conversation
|
@DimitrisJim, thanks for your PR! By analyzing the history of the files in this pull request, we identified @eliben, @berkerpeksag, @warsaw, @brettcannon and @ned-deily to be potential reviewers. |
|
Good catch, changed it. Thanks, Marco! |
Doc/library/pathlib.rst
Outdated
| @@ -652,6 +652,18 @@ call fails (for example because the path doesn't exist): | |||
| .. versionadded:: 3.5 | |||
|
|
|||
|
|
|||
| .. method:: Path.absolute() | |||
|
|
|||
| Return an absolute version of this path. This function works | |||
There was a problem hiding this comment.
Maybe rephrase as "Return an absolute version of this path based on the current working directory"?
Doc/library/pathlib.rst
Outdated
| .. method:: Path.absolute() | ||
|
|
||
| Return an absolute version of this path. This function works | ||
| even if the path doesn't point to anything. |
There was a problem hiding this comment.
"even if the path doesn't exist".
Doc/library/pathlib.rst
Outdated
| Return an absolute version of this path. This function works | ||
| even if the path doesn't point to anything. | ||
|
|
||
| No normalization is done, i.e all '.' and '..' will be kept along. |
There was a problem hiding this comment.
"will be kept along" -> "will not be normalized".
|
Thanks @brettcannon, made the changes you requested. |
There was a problem hiding this comment.
Much better than the method docstring :-) Thanks @brettcannon :-)
|
Thanks! When I have time to also do the cherry-pick PRs I will merge. |
| .. method:: Path.absolute() | ||
|
|
||
| Return an absolute version of this path based on the current working | ||
| directory. This function works even if the path doesn't exist. |
There was a problem hiding this comment.
This function => This method
I dislike "works even if", maybe: "This method doesn't check if the path exists." ?
| @@ -652,6 +652,18 @@ call fails (for example because the path doesn't exist): | |||
| .. versionadded:: 3.5 | |||
|
|
|||
|
|
|||
| .. method:: Path.absolute() | |||
|
|
|||
| Return an absolute version of this path based on the current working | |||
There was a problem hiding this comment.
Hum, maybe specify that only relative paths are resolved using the current working directory?
Return an absolute version of this path. Relative paths are resolved using the current working directory.
There was a problem hiding this comment.
Note that the absolute method is barely tested in test_pathlib. https://github.com/python/cpython/blob/master/Lib/test/test_pathlib.py#L1343 is the only place it's indirectly tested.
There is also the following comment in its implementation:
# FIXME this must defer to the specific flavour (and, under Windows,
# use nt._getfullpathname())https://github.com/python/cpython/blob/master/Lib/pathlib.py#L1096
|
Oh wow, these comments are scary. I would feel more confortable with a change adding more tests and removing "# FIXME this must defer to the specific flavour (and, under Windows, use nt._getfullpathname())" comment. |
|
That would help explain why it isn't documented. 😉 @DimitrisJim are you up for writing some tests for the function? |
|
I'd rather leave the testing for someone more aware of the intricacies of paths for different OS's. Should we close the PR and rename the issue on b.p.o to something like "pathlib.absolute needs tests and documentation"? |
|
Sure, we can close this for now and re-open if we decide to not deprecate Path.absolute(). Thanks for the PR regardless of whether it gets merged. |
No description provided.