-
-
Notifications
You must be signed in to change notification settings - Fork 33.7k
bpo-24899: Add docs for pathlib.Path.absolute and pathlib to os.path correspondence #1753
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
|
@slashfoo, thanks for your PR! By analyzing the history of the files in this pull request, we identified @eliben, @serhiy-storchaka and @marco-buttu to be potential reviewers. |
|
Hello, and thanks for your contribution! I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA). Unfortunately we couldn't find an account corresponding to your GitHub username on bugs.python.org (b.p.o) to verify you have signed the CLA (this might be simply due to a missing "GitHub Name" entry in your b.p.o account settings). This is necessary for legal reasons before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue. Thanks again to your contribution and we look forward to looking at it! |
|
signed the CLA. |
|
@bricsuc can you review these doc changes? |
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should use the code style for '.' and '..'
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.
made the change, thanks for the comment, using double backticks.
Doc/library/pathlib.rst
Outdated
| even if the path doesn't point to anything. | ||
|
|
||
| No normalization is done, i.e. all '.' and '..' will be kept along. | ||
| Use resolve() to get the canonical path to a file. |
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.
:meth:`Path.resolve`
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.
good catch, changed in next commit.
Doc/library/pathlib.rst
Outdated
|
|
||
| .. versionadded:: 3.5 | ||
|
|
||
| Correspondence to tools in the os package |
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.
:mod:`os` module
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.
changed in next commit.
Doc/library/pathlib.rst
Outdated
| ----------------------------------------- | ||
|
|
||
| If you're more familiar with :mod:`os.path` module, here's a correspondence | ||
| table on how the same things may be accomplished with pathlib. |
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.
:mod:`pathlib`
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.
changed in next commit.
| If you're more familiar with :mod:`os.path` module, here's a correspondence | ||
| table on how the same things may be accomplished with pathlib. | ||
|
|
||
| .. note:: |
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 see in this file there is no blank line after .. note::, but this role does not apply to the rest of the doc. Maybe @brettcannon could tell us what is the policy. Another question: what to do in case the policy is to have a blank line? Do we stay consistent with the file or with the general policy?
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 added the blank line, to adhere to general policy, at least with this change.
Doc/library/pathlib.rst
Outdated
|
|
||
| Correspondence to tools in the os package | ||
| ----------------------------------------- | ||
| Correspondence to tools in the :mod:`os` package |
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.
os is a module. You should write: .... tools in the:mod:`os` module
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, I missed that correction from last comment, I thought only the link was missing. Fixing in next commit.
Doc/library/pathlib.rst
Outdated
| methods. Many of these methods can raise an :exc:`OSError` if a system | ||
| call fails (for example because the path doesn't exist): | ||
|
|
||
| .. method:: Path.absolute() |
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.
Please don't document Path.absolute() until bpo-29688 is resolved.
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.
Should we wait, or pluck it from this pull request?
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.
saw the discussion on the linked issue, and I think it's better to pluck it from this one. Doing that on next commit.
Doc/library/pathlib.rst
Outdated
| Correspondence to tools in the :mod:`os` package | ||
| ------------------------------------------------ | ||
|
|
||
| If you're more familiar with :mod:`os.path` module, here's a correspondence |
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.
Below is a table mapping various :mod:`os` functions to their corresponding :class:`PurePath`/:class:`Path` equivalent.
Basically the documentation doesn't take quite such a casual, conversational tone.
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 for the correction, changing that line to the suggested one.
|
|
||
| .. note:: | ||
|
|
||
| Although :func:`os.path.relpath` and :meth:`PurePath.relative_to` have some |
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 would simplify this note to:
Although :func:`os.path.relpath` and :meth:`PurePath.relative_to` have some overlapping use-cases, their semantics differ enough to warrant not considering them equivalent.
If people want to dive deeper they can then read the docs for the two functions.
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.
adopted the suggested line in next commit.
Doc/library/pathlib.rst
Outdated
| ============================ ============================== | ||
| os and os.path pathlib | ||
| ============================ ============================== | ||
| :func:`os.path.abspath` :func:`Path.absolute` |
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.
Should probably change this to Path.resolve.
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.
changed Path.absolute to Path.resolve
Doc/library/pathlib.rst
Outdated
| ============================ ============================== | ||
| :func:`os.path.abspath` :func:`Path.absolute` | ||
| :func:`os.getcwd` :func:`Path.cwd` | ||
| :func:`os.path.abspath` :meth:`Path.absolute` |
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.
Duplicate line.
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.
removed duplicate.
Doc/library/pathlib.rst
Outdated
| :func:`os.path.isfile` :meth:`Path.is_file` | ||
| :func:`os.path.islink` :meth:`Path.is_symlink` | ||
| :func:`os.stat` :meth:`Path.owner` | ||
| :func:`os.stat` :meth:`Path.stat` |
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 would combine the three os.stat lines into one and simply comma-separate the pathlib equivalents. Otherwise someone looking for an os.stat equivalent will find the first solution and then never see the other two.
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.
makes perfect sense, thanks. collapsed the os.stat lines into one.
|
What do you think of the latest version, @marco-buttu ? |
|
@brettcannon it LGTM :-) |
|
@slashfoo this chart took enough effort to warrant an ACKS mention if you want to add yourself to the file before I merge this PR. |
|
Thanks @brettcannon, I did now. I'm happy to be on that list. Now I just need to find more stuff on bpo for me. |
Documentation changes for
pathlibto have a map for people that are more familiar withos.path.Briefly discussed with @brettcannon and @gpshead