-
-
Notifications
You must be signed in to change notification settings - Fork 30.1k
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-22021: Update root_dir and base_dir documentation in shutil #10367
Conversation
…ample in shutil in order to make more clear how they are to be used
Doc/library/shutil.rst
Outdated
@@ -532,11 +532,13 @@ provided. They rely on the :mod:`zipfile` and :mod:`tarfile` modules. | |||
|
|||
*root_dir* is a directory that will be the root directory of the | |||
archive; for example, we typically chdir into *root_dir* before creating the | |||
archive. | |||
archive. This also means that all paths in the archive will be relative to | |||
*root_dir*. |
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.
What do you think of
*root_dir* is a directory that will be the root directory of the
archive, all paths in the archive will be relative to it; for example,
we typically chdir into *root_dir* before creating the archive.
?
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 like it better. I can remember having some difficulties in the wording of this. Thanks for your suggestion.
Thanks for taking the time to improve the documentation, can you have a look as to why the CI is failing? |
…es in the added example
@remilapeyre It turns out it was only a trailing whitespace, which is now fixed. |
@@ -680,6 +681,33 @@ The resulting archive contains: | |||
-rw-r--r-- tarek/staff 37192 2010-02-06 18:23:10 ./known_hosts | |||
|
|||
|
|||
.. _shutil-archiving-example-with-basedir: |
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.
Could you please link to the example from the make_archive()
documentation?
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 @berkerpeksag for the review. From where exactly would you think it would be best to link? From the title itself or from somewhere in the text?
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'm thinking about adding something like "See :ref:`shutil-archiving-example-with-basedir` for how to use *base_dir* and *root_dir* together." after https://github.com/python/cpython/pull/10367/files#diff-bda6196613f849e0aa43c7e2dd9f6068R540
What do you think?
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.
That would be really nice actually. This way someone, who might not scroll all the way down, will not miss the example. I think it may be a better idea to not have a whole new paragraph for the "See the example" bit. It feels more natural to me to just append it at the end of the current paragraph. What is your view on that?
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.
Sounds good to me!
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 |
…the example code, tree output for the directory structure
@berkerpeksag, thanks for the review. I have made the requested changes; please review again. I have included a link to the |
Thanks for making the requested changes! @berkerpeksag: please review the changes made to this pull request. |
@berkerpeksag I have pushed the necessary changes, so the PR might be ready for another (final?) review. |
Sorry, I can't merge this PR. Reason: |
3 similar comments
Sorry, I can't merge this PR. Reason: |
Sorry, I can't merge this PR. Reason: |
Sorry, I can't merge this PR. Reason: |
Thanks @lysnikolaou for the PR 🌮🎉.. I'm working now to backport this PR to: 3.6, 3.7, 3.8, 3.9. |
…onGH-10367) Also added an example in shutil in order to make more clear how they are to be used. Initially reported by Weinan Li on bpo. (cherry picked from commit 7633371) Co-authored-by: Lysandros Nikolaou <[email protected]>
GH-20709 is a backport of this pull request to the 3.9 branch. |
GH-20710 is a backport of this pull request to the 3.8 branch. |
…onGH-10367) Also added an example in shutil in order to make more clear how they are to be used. Initially reported by Weinan Li on bpo. (cherry picked from commit 7633371) Co-authored-by: Lysandros Nikolaou <[email protected]>
…onGH-10367) Also added an example in shutil in order to make more clear how they are to be used. Initially reported by Weinan Li on bpo. (cherry picked from commit 7633371) Co-authored-by: Lysandros Nikolaou <[email protected]>
GH-20711 is a backport of this pull request to the 3.7 branch. |
GH-20712 is a backport of this pull request to the 3.6 branch. |
…onGH-10367) Also added an example in shutil in order to make more clear how they are to be used. Initially reported by Weinan Li on bpo. (cherry picked from commit 7633371) Co-authored-by: Lysandros Nikolaou <[email protected]>
…0367) Also added an example in shutil in order to make more clear how they are to be used. Initially reported by Weinan Li on bpo. (cherry picked from commit 7633371) Co-authored-by: Lysandros Nikolaou <[email protected]>
…0367) Also added an example in shutil in order to make more clear how they are to be used. Initially reported by Weinan Li on bpo. (cherry picked from commit 7633371) Co-authored-by: Lysandros Nikolaou <[email protected]>
…0367) Also added an example in shutil in order to make more clear how they are to be used. Initially reported by Weinan Li on bpo. (cherry picked from commit 7633371) Co-authored-by: Lysandros Nikolaou <[email protected]>
…onGH-10367) Also added an example in shutil in order to make more clear how they are to be used. Initially reported by Weinan Li on bpo.
Also added an example in shutil in order to make more clear how they are to be used.
Initially reported by Weinan Li on bpo.
https://bugs.python.org/issue22021
Automerge-Triggered-By: @gvanrossum