Uh oh!
There was an error while loading. Please reload this page.
bpo-29688: Document Path.absolute - #384
Conversation
mention-bot
commented
Mar 1, 2017
@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. |
DimitrisJim
commented
Mar 1, 2017
Good catch, changed it. Thanks, Marco! |
| .. 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"?
| .. 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".
| 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".
DimitrisJim
commented
Mar 1, 2017
Thanks @brettcannon, made the changes you requested. |
marco-buttu
left a comment
There was a problem hiding this comment.
Much better than the method docstring :-) Thanks @brettcannon :-)
brettcannon
commented
Mar 1, 2017
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." ?
| .. 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.
berkerpeksag
left a comment
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
vstinner
commented
Mar 3, 2017
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. |
brettcannon
commented
Mar 3, 2017
That would help explain why it isn't documented. 😉 @DimitrisJim are you up for writing some tests for the function? |
DimitrisJim
commented
Mar 3, 2017
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"? |
brettcannon
commented
Mar 3, 2017
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.