Skip to content

/ cpython

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

[3.8] Fix typos in Misc/NEWS.d #17930

Open
wants to merge 2 commits into
base: 3.8
from
Open

[3.8] Fix typos in Misc/NEWS.d #17930

wants to merge 2 commits into from
+6 −6

Conversation

@toonarmycaptain
Copy link
Contributor

toonarmycaptain commented Jan 9, 2020

With reference to corrections to news in 3.9.0a2.rst backporting some typo corrections to 3.8.0/3.8.1.
@toonarmycaptain
Backport typo corrections.
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
279c027
@toonarmycaptain
Update 3.8.0.rst
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
891a8ae
@aeros
aeros suggested changes Jan 9, 2020
View changes
Copy link
Member

aeros left a comment

Thanks for the PR @toonarmycaptain.

Could you prefix the PR title with [3.8]? This would look like: [3.8] Backport typo corrections. This format is required for any PR opened against a maintenance branch, otherwise it won't pass @bedevere-bot's checks.

I would also consider adjusting to title to something more informative of the PR, such as [3.8] Fix typos in Misc/NEWS.d. "typo corrections" doesn't explain much about the location of the changes, as that could apply to any part of the documentation, code comments, docstrings, etc.

I believe it is correct to specifically open the PR against the 3.8 branch, as changes to Misc/NEWS.d are typically only applied to branch's versions rather than all versions. This differs from other areas, where the changes should be applied to master and then backported to previous versions to all relevant locations.

I'm not certain about the typo fix that adjusts the number of spaces between sentences. IIRC, it still renders exactly the same in reST; @merwok or @willingc might be able to clarify on that.

The other changes LGTM though, thanks!
@toonarmycaptain toonarmycaptain changed the title Backport typo corrections. [3.8] Fix typos in Misc/NEWS.d Jan 10, 2020
@toonarmycaptain

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

toonarmycaptain commented Jan 10, 2020

Title change done.
I can remove the space change if you like? It was just inconsistent, so I changed it. I made the PR referenced above against 3.9, and was just trying to find the corrections from that PR and bring them to 3.8 (so that they actually change in the 3.8 docs), as like you'd said I'd expected it to carry back.
@aeros

This comment has been minimized.

Sign in to view
Copy link
Member

aeros commented Jan 11, 2020

@toonarmycaptain

Title change done.

Thanks.

I can remove the space change if you like? It was just inconsistent, so I changed it.

You can wait for additional feedback on that, but generally speaking we try to avoid making changes purely for the sake of consistency. This is significantly more important for changes to the source code, but it also applies to documentation or news changes. That's why I @ mentioned a couple of core developers that were especially experienced with reST, to check if the number of spaces makes a difference there.

One small unnecessary change may not seem like a big deal, but it can really add up over time. PR review time comes at a premium, especially for a project ran almost entirely by voluntary efforts. It improves our workflow significantly when PRs have a more focused scope and avoid any unneeded changes.

That being said, there's no harm at all in waiting for a second opinion; we have to wait on approval from a core developer before it can be merged.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aeros aeros

Assignees
No one assigned
Labels
CLA signed awaiting core review skip issue skip news type-documentation
Projects
None yet
Milestone
No milestone
4 participants
@toonarmycaptain @aeros @the-knights-who-say-ni @bedevere-bot
You can’t perform that action at this time.