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.

Sign up for GitHub

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

Jump to bottom

bpo-30840: Document relative imports #12831

Merged
merged 13 commits into from
Apr 24, 2019
Merged

bpo-30840: Document relative imports #12831

merged 13 commits into from
Apr 24, 2019

Conversation

nanjekyejoannah
Copy link
Contributor

@nanjekyejoannah nanjekyejoannah commented Apr 14, 2019 •
edited by bedevere-bot
Loading

@bedevere-bot bedevere-bot added docs Documentation in the Doc dir awaiting review labels Apr 14, 2019
@nanjekyejoannah
Copy link
Contributor Author

cc @csabella for review.

pganssle
pganssle reviewed Apr 14, 2019
View reviewed changes
Copy link
Member

@pganssle pganssle left a comment

Choose a reason for hiding this comment

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

Some editing notes. I have marked this review as "Comment" rather than "Request changes", because I did not see any problems with the grammar, and my notes are mainly stylistic.

Doc/reference/import.rst Outdated Show resolved Hide resolved
Doc/reference/import.rst Outdated Show resolved Hide resolved
Doc/reference/import.rst Outdated Show resolved Hide resolved
mhsmith
mhsmith reviewed Apr 15, 2019
View reviewed changes
Doc/reference/import.rst Outdated Show resolved Hide resolved
@nanjekyejoannah
Copy link
Contributor Author

@pganssle and @mhsmith done. I hope I have taken care of your review.

pganssle
pganssle approved these changes Apr 17, 2019
View reviewed changes
Copy link
Member

@pganssle pganssle left a comment •
edited
Loading

Choose a reason for hiding this comment

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

No more style and grammar issues from my end, though I'll defer to someone else about whether or not this solves the bpo issue (@ncoghlan maybe?)

Thanks!

ncoghlan
ncoghlan requested changes Apr 19, 2019
View reviewed changes
Copy link
Contributor

@ncoghlan ncoghlan left a comment

Choose a reason for hiding this comment

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

The new section looks good (thanks!), but to resolve the original documentation bug, the PEP 328 cross-reference in https://docs.python.org/3/reference/simple_stmts.html#the-import-statement needs to be updated to link to this new section instead.

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

@nanjekyejoannah
Copy link
Contributor Author

nanjekyejoannah commented Apr 21, 2019 •
edited
Loading

I have made the requested changes; please review again .

@brettcannon
Copy link
Member

@nanjekyejoannah Do note that you need to reply with I have made the requested changes; please review again to trigger our bot to recognize that you're ready for a new review.

@nanjekyejoannah
Copy link
Contributor Author

@brettcannon Noted and thanks!

@brettcannon brettcannon reopened this Apr 23, 2019
@brettcannon brettcannon requested review from ncoghlan and removed request for brettcannon April 23, 2019 00:38
@brettcannon
Copy link
Member

@nanjekyejoannah just so you know for future PRs, the magic phrase needs to be in a new comment, otherwise the bot won't pick it up. But I have gone ahead and manually changed the label and requested @ncoghlan to do another review.

ncoghlan
ncoghlan approved these changes Apr 24, 2019
View reviewed changes
Copy link
Contributor

@ncoghlan ncoghlan left a comment

Choose a reason for hiding this comment

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

Thanks! I was going to suggest adding a hyphen as part of the new cross-reference label, but we're currently missing one of those in the importsystem target as well, so leaving it out is consistent within this particular file.

@ncoghlan ncoghlan merged commit 70bf713 into python:master Apr 24, 2019
@bedevere-bot
Copy link

@ncoghlan: Please replace # with GH- in the commit message next time. Thanks!

@miss-islington
Copy link
Contributor

Thanks @nanjekyejoannah for the PR, and @ncoghlan for merging it 🌮🎉.. I'm working now to backport this PR to: 3.7.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Apr 24, 2019
@nanjekyejoannah @miss-islington
bpo-30840: Document relative imports (pythonGH-12831)
4267905 
* document relative imports

* 📜🤖 Added by blurb_it.

* fix indentation error

* remove indentation

* Document relative imports

* Document relative imports

* remove from ...package

* Document relative imports

* remove trailing space

* Document relative imports

* Document relative imports
(cherry picked from commit 70bf713)

Co-authored-by: Joannah Nanjekye <[email protected]>
@bedevere-bot
Copy link

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

@ncoghlan
Copy link
Contributor

Hmm, I just had the GitHub commit message editor break on me again:

  • told me the squash & merge had failed
  • actually went ahead and squashed and merged anyway, but didn't include my commit message changes

@nanjekyejoannah nanjekyejoannah deleted the issue30840 branch April 24, 2019 15:18
ncoghlan pushed a commit that referenced this pull request Apr 24, 2019
@miss-islington @nanjekyejoannah @ncoghlan
bpo-30840: Document relative imports (GH-12831) (GH-12938)
4d0233e 
* Document relative imports
(cherry picked from commit 70bf713)

Co-authored-by: Joannah Nanjekye <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mhsmith mhsmith mhsmith left review comments

@ncoghlan ncoghlan ncoghlan approved these changes

@pganssle pganssle pganssle approved these changes

Assignees
No one assigned
Labels
docs Documentation in the Doc dir
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@nanjekyejoannah @bedevere-bot @brettcannon @miss-islington @ncoghlan @mhsmith @pganssle @the-knights-who-say-ni