Skip to content
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

Merged
merged 5 commits into from
Jun 8, 2020

Conversation

lysnikolaou
Copy link
Contributor

@lysnikolaou lysnikolaou commented Nov 6, 2018

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

…ample in shutil in order to make more clear how they are to be used
Doc/library/shutil.rst Outdated Show resolved Hide resolved
@@ -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*.
Copy link
Contributor

@remilapeyre remilapeyre Mar 4, 2019

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.

?

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 like it better. I can remember having some difficulties in the wording of this. Thanks for your suggestion.

Doc/library/shutil.rst Outdated Show resolved Hide resolved
@remilapeyre
Copy link
Contributor

Thanks for taking the time to improve the documentation, can you have a look as to why the CI is failing?

https://travis-ci.org/python/cpython/jobs/501463292

@lysnikolaou
Copy link
Contributor Author

@remilapeyre It turns out it was only a trailing whitespace, which is now fixed.

Doc/library/shutil.rst Outdated Show resolved Hide resolved
Doc/library/shutil.rst Outdated Show resolved Hide resolved
Doc/library/shutil.rst Outdated Show resolved Hide resolved
Doc/library/shutil.rst Outdated Show resolved Hide resolved
Doc/library/shutil.rst Outdated Show resolved Hide resolved
@@ -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:
Copy link
Member

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?

Copy link
Contributor Author

@lysnikolaou lysnikolaou May 13, 2019

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?

Copy link
Member

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?

Copy link
Contributor Author

@lysnikolaou lysnikolaou May 13, 2019

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?

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good to me!

@bedevere-bot
Copy link

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 I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

…the example code, tree output for the directory structure
@lysnikolaou
Copy link
Contributor Author

@berkerpeksag, thanks for the review. I have made the requested changes; please review again. I have included a link to the make_archive example in the text, but I'm not really sure that's what you had in mind.

@bedevere-bot
Copy link

Thanks for making the requested changes!

@berkerpeksag: please review the changes made to this pull request.

@lysnikolaou
Copy link
Contributor Author

@berkerpeksag I have pushed the necessary changes, so the PR might be ready for another (final?) review.

@miss-islington miss-islington merged commit 7633371 into python:master Jun 8, 2020
@miss-islington
Copy link
Contributor

Sorry, I can't merge this PR. Reason: Base branch was modified. Review and try the merge again..

3 similar comments
@miss-islington
Copy link
Contributor

Sorry, I can't merge this PR. Reason: Base branch was modified. Review and try the merge again..

@miss-islington
Copy link
Contributor

Sorry, I can't merge this PR. Reason: Base branch was modified. Review and try the merge again..

@miss-islington
Copy link
Contributor

Sorry, I can't merge this PR. Reason: Base branch was modified. Review and try the merge again..

@miss-islington
Copy link
Contributor

Thanks @lysnikolaou for the PR 🌮🎉.. I'm working now to backport this PR to: 3.6, 3.7, 3.8, 3.9.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jun 8, 2020
…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]>
@bedevere-bot bedevere-bot removed the needs backport to 3.9 only security fixes label Jun 8, 2020
@bedevere-bot
Copy link

GH-20709 is a backport of this pull request to the 3.9 branch.

@bedevere-bot
Copy link

GH-20710 is a backport of this pull request to the 3.8 branch.

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jun 8, 2020
…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]>
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jun 8, 2020
…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]>
@bedevere-bot
Copy link

GH-20711 is a backport of this pull request to the 3.7 branch.

@bedevere-bot
Copy link

GH-20712 is a backport of this pull request to the 3.6 branch.

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jun 8, 2020
…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]>
miss-islington added a commit that referenced this pull request Jun 8, 2020
…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]>
miss-islington added a commit that referenced this pull request Jun 8, 2020
…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]>
miss-islington added a commit that referenced this pull request Jun 8, 2020
…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]>
arun-mani-j pushed a commit to arun-mani-j/cpython that referenced this pull request Jul 21, 2020
…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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Documentation in the Doc dir skip news
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants