Skip to content

Conversation

@andresdelfino
Copy link
Contributor

No description provided.

@bedevere-bot bedevere-bot added docs Documentation in the Doc dir awaiting review labels Sep 1, 2019
@corona10 corona10 requested a review from vstinner September 2, 2019 01:43
@corona10
Copy link
Member

corona10 commented Sep 2, 2019

@vstinner We can add a skip news or skip issue label for this kind of PR?

Copy link
Contributor

@aeros aeros left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @andresdelfino!

@vstinner We can add a skip news or skip issue label for this kind of PR?

The changes look good to me, and this PR doesn't require an issue or news entry since it's just involving a typo correction. There are other minor changes which can use skip news and skip issue labels, but this one is a perfect example.

Copy link
Member

@corona10 corona10 left a comment

Choose a reason for hiding this comment

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

@aeros167 Thanks for the mentoring :)
This PR looks good to me also.

Copy link
Contributor

@aeros aeros left a comment

Choose a reason for hiding this comment

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

Although I approve of the changes made, I just realized that this will cause the length of line 1256 to go beyond the reST maximum (https://devguide.python.org/documenting/#use-of-whitespace):

All reST files use an indentation of 3 spaces; no tabs are allowed. The maximum line length is 80 characters for normal text

We'll have to trim down this line to be at or below 80 chars before merging it. An easy way to measure the length is by copy/pasting the text into the python interpreter and using the builtin len() function. There are some areas of the documentation that exceed the 80 char line limit, but it would be preferable if we could fix this one before merging it.

@aeros
Copy link
Contributor

aeros commented Sep 2, 2019

@aeros167 Thanks for the mentoring :)

No problem! Feel free to @mention me if you have any general triaging or documentation questions. @vstinner is far more knowledgeable, but I should be able to help with more simple issues.

@vstinner
Copy link
Member

vstinner commented Sep 2, 2019

@corona10: "skip news" is fine for documentation-only changes. "skip issue" is ok-ish: I don't think that it's worth it to bother with an issue to track backports. Please add "needs 3.7 backport" and "needs 3.8 backport" labels: I like to continue to update the 3.7 doc, but I don't think that it's worth it to update 2.7 doc. The reference for branches open for bugfixes: https://devguide.python.org/#status-of-python-branches

Co-Authored-By: Kyle Stanley <aeros167@gmail.com>
@aeros
Copy link
Contributor

aeros commented Sep 2, 2019

@vstinner

"skip news" is fine for documentation-only changes.

IMO, typo corrections. clarifications, or other minor improvements are definitely appropriate for a skip news label, but I think it can be beneficial to have a news entry when there's a substantial new section added. From what Brett has told me in the past, it's more of an "opt-out" system where it's up to the author, but I think we should encourage news entries for the more substantial documentation changes. It certainly doesn't hurt.

I don't think that it's worth it to update 2.7 doc.

Since we're approaching EoL for 2, are we generally avoiding backports for documentation changes for 2.7? I completely agree that it's probably not worthwhile, just wanted to check if there was an official stance on this topic.

@vstinner
Copy link
Member

vstinner commented Sep 4, 2019

Since we're approaching EoL for 2, are we generally avoiding backports for documentation changes for 2.7? I completely agree that it's probably not worthwhile, just wanted to check if there was an official stance on this topic.

In my experience, backports from master to 2.7 branch are painful: lot of conflicts. IMHO it's not worth it.

I'm not aware of an official stance.

@ned-deily
Copy link
Member

ned-deily commented Sep 4, 2019

I think we should encourage news entries for the more substantial documentation changes. It certainly doesn't hurt.

It doesn't hurt but I can think of few instances when it is really helpful to have a NEWS entry for a doc-only change, especially in a case where the change is only grammatical or formatting. A NEWS entry might be appropriate in a case where, say, the original doc entry was just flat-out wrong and had been the cause of users' queries. The intent of NEWS entries is to give a quick hint to problems that have been fixed and new features that have been added to the code base. It is not meant to document every change in a release; people interested in that level of detail can easily peruse the git change log on GitHub.

I also agree that, in general, we should not be backporting changes to 2.7 at this point. Users have been living with 2.7's quirks for ten years now.

@andresdelfino
Copy link
Contributor Author

@aeros167 could you review the change I made after your request?

@aeros
Copy link
Contributor

aeros commented Sep 5, 2019

It doesn't hurt but I can think of few instances when it is really helpful to have a NEWS entry for a doc-only change, especially in a case where the change is only grammatical or formatting. A NEWS entry might be appropriate in a case where, say, the original doc entry was just flat-out wrong and had been the cause of users' queries.

Yeah I was mostly referring to changes that made significant modifications to existing sections, which would change the meaning or significantly increase the amount of information provided. Grammatical or formatting changes definitely wouldn't need news entries.

In my experience, backports from master to 2.7 branch are painful: lot of conflicts. IMHO it's not worth it.

I also agree that, in general, we should not be backporting changes to 2.7 at this point. Users have been living with 2.7's quirks for ten years now.

Thanks for the clarification @vstinner and @ned-deily. Even if it's not an official stance, it's still very useful to have a rough idea of where we stand on 2.7 backports.

@vstinner vstinner merged commit 3038e87 into python:master Sep 5, 2019
@miss-islington
Copy link
Contributor

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

@miss-islington
Copy link
Contributor

I'm having trouble backporting to 3.8. Reason: 'Error 110 while writing to socket. Connection timed out.'. Please retry by removing and re-adding the needs backport to 3.8 label.

@bedevere-bot
Copy link

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

@miss-islington
Copy link
Contributor

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

@miss-islington
Copy link
Contributor

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

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Sep 5, 2019
(cherry picked from commit 3038e87)

Co-authored-by: Andre Delfino <adelfino@gmail.com>
@bedevere-bot
Copy link

GH-15698 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 Sep 5, 2019
(cherry picked from commit 3038e87)

Co-authored-by: Andre Delfino <adelfino@gmail.com>
miss-islington added a commit that referenced this pull request Sep 5, 2019
(cherry picked from commit 3038e87)

Co-authored-by: Andre Delfino <adelfino@gmail.com>
@miss-islington
Copy link
Contributor

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

@bedevere-bot
Copy link

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

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Sep 5, 2019
(cherry picked from commit 3038e87)

Co-authored-by: Andre Delfino <adelfino@gmail.com>
@vstinner
Copy link
Member

vstinner commented Sep 5, 2019

AppVeyor job didn't run on the first 3.7 backport PR. So I closed it and created a new one.

miss-islington added a commit that referenced this pull request Sep 5, 2019
(cherry picked from commit 3038e87)

Co-authored-by: Andre Delfino <adelfino@gmail.com>
@andresdelfino andresdelfino deleted the patch-1 branch September 6, 2019 01:32
lisroach pushed a commit to lisroach/cpython that referenced this pull request Sep 10, 2019
DinoV pushed a commit to DinoV/cpython that referenced this pull request Jan 14, 2020
websurfer5 pushed a commit to websurfer5/cpython that referenced this pull request Jul 20, 2020
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 issue skip news

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants