bpo-37284: Add note to sys.implementation doc by potomak · Pull Request #14328 · python/cpython

potomak · 2019-06-24T00:50:11Z

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 · 2019-06-24T00:50:13Z

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!

aeros

@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 · 2019-07-07T17:06:54Z

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 · 2019-07-09T13:54:58Z

cc @ericsnowcurrently

ericsnowcurrently

Thanks for working on this!

LGTM

I just have one small recommended tweak.

ericsnowcurrently

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 · 2019-07-12T15:59:15Z

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 · 2019-07-12T19:56:21Z

@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 · 2019-07-12T20:34:52Z

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

aeros · 2019-07-12T20:55:49Z

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.

@ericsnowcurrently

Add @ericsnowcurrently's suggestion. Co-Authored-By: Eric Snow <ericsnowcurrently@gmail.com>

potomak · 2019-07-13T13:06:46Z

I have made the requested changes; please review again

bedevere-bot · 2019-07-13T13:06:49Z

Thanks for making the requested changes!

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

ericsnowcurrently · 2019-07-15T14:36:04Z

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

ericsnowcurrently

LGTM

Thanks for doing this!

miss-islington · 2019-07-15T14:38:38Z

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

miss-islington · 2019-07-15T14:38:39Z

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

bedevere-bot · 2019-07-15T14:38:50Z

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

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 · 2019-07-15T14:38:57Z

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

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 · 2019-07-15T21:41:58Z

@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! (:

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 added the CLA not signed label Jun 24, 2019

bedevere-bot added docs Documentation in the Doc dir awaiting review labels Jun 24, 2019

aeros suggested changes Jul 6, 2019

View reviewed changes

bedevere-bot added awaiting core review and removed awaiting review labels Jul 6, 2019

the-knights-who-say-ni added CLA signed and removed CLA not signed labels Jul 7, 2019

Add note to sys.implementation doc

b68415d

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

potomak force-pushed the patch-1 branch from 11ce683 to b68415d Compare July 7, 2019 17:03

ericsnowcurrently self-requested a review July 9, 2019 16:51

ericsnowcurrently approved these changes Jul 12, 2019

View reviewed changes

Comment thread Doc/library/sys.rst Outdated

bedevere-bot added awaiting merge and removed awaiting core review labels Jul 12, 2019

ericsnowcurrently requested changes Jul 12, 2019

View reviewed changes

bedevere-bot removed the awaiting merge label Jul 12, 2019

bedevere-bot added the awaiting changes label Jul 12, 2019

potomak and others added 2 commits July 13, 2019 08:48

Update Doc/library/sys.rst

083277f

Add @ericsnowcurrently's suggestion. Co-Authored-By: Eric Snow <ericsnowcurrently@gmail.com>

📜🤖 Added by blurb_it.

2543ac0

bedevere-bot added awaiting change review and removed awaiting changes labels Jul 13, 2019

ericsnowcurrently added the 🤖 automerge label Jul 15, 2019

ericsnowcurrently approved these changes Jul 15, 2019

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting change review labels Jul 15, 2019

miss-islington merged commit 52693c1 into python:master Jul 15, 2019

bedevere-bot removed the awaiting merge label Jul 15, 2019

ericsnowcurrently added needs backport to 3.7 labels Jul 15, 2019

bedevere-bot removed the needs backport to 3.8 label Jul 15, 2019

bedevere-bot removed the needs backport to 3.7 label Jul 15, 2019

potomak deleted the patch-1 branch July 16, 2019 02:14

lisroach pushed a commit to lisroach/cpython that referenced this pull request Sep 10, 2019

bpo-37284: Add note to sys.implementation doc (pythonGH-14328)

7bb03b9

Add a brief note to indicate that any new required attributes must go through the PEP process. https://bugs.python.org/issue37284

DinoV pushed a commit to DinoV/cpython that referenced this pull request Jan 14, 2020

bpo-37284: Add note to sys.implementation doc (pythonGH-14328)

ff03b1e

Add a brief note to indicate that any new required attributes must go through the PEP process. https://bugs.python.org/issue37284

Uh oh!

Conversation

potomak commented Jun 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

aeros left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

potomak commented Jul 7, 2019

Uh oh!

nanjekyejoannah commented Jul 9, 2019

Uh oh!

ericsnowcurrently left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ericsnowcurrently left a comment

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Jul 12, 2019

Uh oh!

aeros commented Jul 12, 2019

Uh oh!

ericsnowcurrently commented Jul 12, 2019

Uh oh!

aeros commented Jul 12, 2019

Uh oh!

potomak commented Jul 13, 2019

Uh oh!

bedevere-bot commented Jul 13, 2019

Uh oh!

ericsnowcurrently commented Jul 15, 2019

Uh oh!

ericsnowcurrently left a comment

Choose a reason for hiding this comment

Uh oh!

miss-islington commented Jul 15, 2019

Uh oh!

miss-islington commented Jul 15, 2019

Uh oh!

bedevere-bot commented Jul 15, 2019

Uh oh!

bedevere-bot commented Jul 15, 2019

Uh oh!

aeros commented Jul 15, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

potomak commented Jun 24, 2019 •

edited

Loading

aeros left a comment •

edited

Loading