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-37284: Add note to sys.implementation doc #14328

Merged
merged 3 commits into from Jul 15, 2019
Merged

Conversation

@potomak
Copy link
Contributor

potomak commented Jun 24, 2019

Add a brief note to indicate that any new required attributes must go through the PEP process.

https://bugs.python.org/issue37284

@the-knights-who-say-ni

This comment has been minimized.

Copy link

the-knights-who-say-ni commented Jun 24, 2019

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

Our records indicate we have not received your CLA. For legal reasons we need you to sign this before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

If you have recently signed the CLA, please wait at least one business day
before our records are updated.

You can check yourself to see if the CLA has been received.

Thanks again for your contribution, we look forward to reviewing it!

Copy link
Member

aeros left a comment

@potomak Thanks for the contribution! If you've already signed the CLA and it has been over 24 hours, try manually refreshing on status on GitHub (click on the "check yourself" link posted by the CLA bot @the-knights-who-say-ni). For mine, it was not automatically updated.

Usually when the tests are failing on documentation related PRs, it is a result of whitespace issues. You can attempt to correct this through GitHub's text editor, but the more reliable method is to locally clone the cpython repository using git clone, make your changes to Doc/library/sys.rst (or download Doc/library/sys.rst from your patch-1 branch into your local repo), use git add to add it to the checkout list, and then use the patchcheck utility (varies based on OS, let me know if you have further questions) to automatically correct whitespace issues. More details on this process can be found on the dev guide: https://devguide.python.org/pullrequest/?highlight=patchcheck#patchcheck

Add a brief note to indicate that any new required attributes must go through the PEP process.
@potomak potomak force-pushed the potomak:patch-1 branch from 11ce683 to b68415d Jul 7, 2019
@potomak

This comment has been minimized.

Copy link
Contributor Author

potomak commented Jul 7, 2019

Thanks @aeros167 for your help.

I checked myself for the SLA signature and I updated the commit after running make patchcheck as you suggested.

@nanjekyejoannah

This comment has been minimized.

Copy link
Contributor

nanjekyejoannah commented Jul 9, 2019

@ericsnowcurrently ericsnowcurrently self-requested a review Jul 9, 2019
Copy link
Member

ericsnowcurrently left a comment

Thanks for working on this!

LGTM

I just have one small recommended tweak.

Doc/library/sys.rst Outdated Show resolved Hide resolved
Copy link
Member

ericsnowcurrently left a comment

Oh, and add the missing NEWS entry. You should use the "blurb" tool.

See: https://devguide.python.org/committing/?highlight=blurb#what-s-new-and-news-entries

@bedevere-bot

This comment has been minimized.

Copy link

bedevere-bot commented Jul 12, 2019

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.

@aeros

This comment has been minimized.

Copy link
Member

aeros commented Jul 12, 2019

@ericsnowcurrently So in a case such as this where it's adding a note to the docs justifies adding news entry?

I'm curious since I've been interested in helping with the label management on PRs with the upcoming triager role so I wanted to make sure I have a solid understanding of the appropriate usage of skip news. There's currently no specifications on it over on the devguide.

@ericsnowcurrently

This comment has been minimized.

Copy link
Member

ericsnowcurrently commented Jul 12, 2019

@aeros167, we definitely add NEWS entries for documentation. Documentation is dedicated section. :)

@aeros

This comment has been minimized.

Copy link
Member

aeros commented Jul 12, 2019

we definitely add NEWS entries for documentation. Documentation is dedicated section. :)

@ericsnowcurrently Oh no I didn't mean for documentation in general.

My initial thought was that because this PR is only adding a note to an already existing section that it would justify the usage of a skip news label. So my question was asking whether or not this would apply more broadly to other PRs as well. In general, it can be a little bit difficult to tell whether or not a skip news label is appropriate. Usually my "rule of thumb" has been that if the message conveyed is at all changed, then it should have a news entry.

I've added news entries before when modifying sections in the docs, such as in this PR where I added the exceptions to the os.chdir() summary: #14611.

potomak and others added 2 commits Jul 13, 2019
Add @ericsnowcurrently's suggestion.

Co-Authored-By: Eric Snow <ericsnowcurrently@gmail.com>
@potomak

This comment has been minimized.

Copy link
Contributor Author

potomak commented Jul 13, 2019

I have made the requested changes; please review again

@bedevere-bot

This comment has been minimized.

Copy link

bedevere-bot commented Jul 13, 2019

Thanks for making the requested changes!

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

@ericsnowcurrently

This comment has been minimized.

Copy link
Member

ericsnowcurrently commented Jul 15, 2019

@aeros167, the tricky thing is that small doc changes can have a real impact on someone out there, even an impact that you or I may not realize. Since there isn't much cost to adding a NEWS entry, it is worth doing even for seemingly innocuous changes. Basically the only time I use the skip news label is for PRs that are already covered by a NEWS entry for an earlier PR.

All that said, it would definitely be worth clarifying in the devguide. So feel free to add a PR there (and add me as a reviewer). You can also start a thread on the core-workflow list if you want to make sure you have the directions right. I certainly don't speak for everyone. :)

Copy link
Member

ericsnowcurrently left a comment

LGTM

Thanks for doing this!

@miss-islington miss-islington merged commit 52693c1 into python:master Jul 15, 2019
5 checks passed
5 checks passed
Azure Pipelines PR #20190713.28 succeeded
Details
bedevere/issue-number Issue number 37284 found
Details
bedevere/news News entry found in Misc/NEWS.d
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@miss-islington

This comment has been minimized.

Copy link

miss-islington commented Jul 15, 2019

Thanks @potomak for the PR 🌮🎉.. I'm working now to backport this PR to: 3.8.
🐍🍒🤖

@miss-islington

This comment has been minimized.

Copy link

miss-islington commented Jul 15, 2019

Thanks @potomak for the PR 🌮🎉.. I'm working now to backport this PR to: 3.7.
🐍🍒🤖

@bedevere-bot

This comment has been minimized.

Copy link

bedevere-bot commented Jul 15, 2019

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

miss-islington added a commit to miss-islington/cpython that referenced this pull request Jul 15, 2019
Add a brief note to indicate that any new required attributes must go through the PEP process.

https://bugs.python.org/issue37284
(cherry picked from commit 52693c1)

Co-authored-by: Giovanni Cappellotto <gcappellotto@fb.com>
miss-islington added a commit to miss-islington/cpython that referenced this pull request Jul 15, 2019
Add a brief note to indicate that any new required attributes must go through the PEP process.

https://bugs.python.org/issue37284
(cherry picked from commit 52693c1)

Co-authored-by: Giovanni Cappellotto <gcappellotto@fb.com>
@bedevere-bot

This comment has been minimized.

Copy link

bedevere-bot commented Jul 15, 2019

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

miss-islington added a commit that referenced this pull request Jul 15, 2019
Add a brief note to indicate that any new required attributes must go through the PEP process.

https://bugs.python.org/issue37284
(cherry picked from commit 52693c1)

Co-authored-by: Giovanni Cappellotto <gcappellotto@fb.com>
miss-islington added a commit that referenced this pull request Jul 15, 2019
Add a brief note to indicate that any new required attributes must go through the PEP process.

https://bugs.python.org/issue37284
(cherry picked from commit 52693c1)

Co-authored-by: Giovanni Cappellotto <gcappellotto@fb.com>
@aeros

This comment has been minimized.

Copy link
Member

aeros commented Jul 15, 2019

@ericsnowcurrently Thanks for the clarification, I appreciate it and I can definitely see where you're coming from. Since there's no real cost associated with adding the news entry, I can understand why it would be preferable to always do so when possible. Especially if it involves anything outward facing such as the docs or a public function.

All that said, it would definitely be worth clarifying in the devguide. So feel free to add a PR there (and add me as a reviewer). You can also start a thread on the core-workflow list if you want to make sure you have the directions right. I certainly don't speak for everyone. :)

I've been working on helping with the new GitHub triager role documentation, currently we're still in the early stages. The next component will be adding a summary explaining the general usage for each of the labels. I figured that the skip news was one that was particularly important to get right, so I wanted to get clarification from the core devs as to when they think it's most appropriate. In the PR for adding in the labels section, I'll @ mention you (I don't think that I have the ability to add anyone as a reviewer as a contributor, currently only GitHub Python organization members can do that).

Thanks! (:

@potomak potomak deleted the potomak:patch-1 branch Jul 16, 2019
LorenzMende added a commit to LorenzMende/cpython that referenced this pull request Aug 11, 2019
Add a brief note to indicate that any new required attributes must go through the PEP process.





https://bugs.python.org/issue37284
lisroach added a commit to lisroach/cpython that referenced this pull request Sep 10, 2019
Add a brief note to indicate that any new required attributes must go through the PEP process.





https://bugs.python.org/issue37284
DinoV added a commit to DinoV/cpython that referenced this pull request Jan 14, 2020
Add a brief note to indicate that any new required attributes must go through the PEP process.





https://bugs.python.org/issue37284
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.