Skip to content

Conversation

@slashfoo
Copy link
Contributor

Documentation changes for pathlib to have a map for people that are more familiar with os.path.

Briefly discussed with @brettcannon and @gpshead

@mention-bot
Copy link

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

@the-knights-who-say-ni
Copy link

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!

@slashfoo
Copy link
Contributor Author

signed the CLA.

@brettcannon brettcannon self-requested a review May 23, 2017 17:34
@gpshead
Copy link
Member

gpshead commented May 24, 2017

@bricsuc can you review these doc changes?

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.
Copy link
Contributor

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

Copy link
Contributor Author

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.

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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:meth:`Path.resolve`

Copy link
Contributor Author

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.


.. versionadded:: 3.5

Correspondence to tools in the os package
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:mod:`os` module

Copy link
Contributor Author

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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:mod:`pathlib`

Copy link
Contributor Author

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::
Copy link
Contributor

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?

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 added the blank line, to adhere to general policy, at least with this change.


Correspondence to tools in the os package
-----------------------------------------
Correspondence to tools in the :mod:`os` package
Copy link
Contributor

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

Copy link
Contributor Author

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.

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()
Copy link
Member

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.

Copy link
Contributor Author

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?

Copy link
Contributor Author

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.

Correspondence to tools in the :mod:`os` package
------------------------------------------------

If you're more familiar with :mod:`os.path` module, here's a correspondence
Copy link
Member

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.

Copy link
Contributor Author

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
Copy link
Member

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.

Copy link
Contributor Author

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.

============================ ==============================
os and os.path pathlib
============================ ==============================
:func:`os.path.abspath` :func:`Path.absolute`
Copy link
Member

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.

Copy link
Contributor Author

@slashfoo slashfoo May 26, 2017

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

============================ ==============================
:func:`os.path.abspath` :func:`Path.absolute`
:func:`os.getcwd` :func:`Path.cwd`
:func:`os.path.abspath` :meth:`Path.absolute`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Duplicate line.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed duplicate.

: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`
Copy link
Member

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.

Copy link
Contributor Author

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.

@brettcannon
Copy link
Member

What do you think of the latest version, @marco-buttu ?

@marco-buttu
Copy link
Contributor

@brettcannon it LGTM :-)

@brettcannon
Copy link
Member

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

@brettcannon brettcannon self-assigned this May 28, 2017
@slashfoo
Copy link
Contributor Author

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.

@brettcannon brettcannon merged commit ae8750b into python:master Jun 2, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants