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

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

Merged
merged 4 commits into from Jan 12, 2020
Merged

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

merged 4 commits into from Jan 12, 2020

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.

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.

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.

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.

Reverse deletion of extra space between sentences added solely for consistency.
Reverse deletion of extra space between sentences added solely for consistency.

...apparently there were three spaces originally?
@aeros
aeros approved these changes Jan 12, 2020
@toonarmycaptain

This comment has been minimized.

Copy link
Contributor Author

toonarmycaptain commented Jan 12, 2020

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.

No problem, no need to bother the core devs unnecessarily! I just left it up to you as you were reviewing.

Looking at https://docs.python.org/3.8/whatsnew/changelog.html#python-3-8-1-final, and this SO question, there's one space in my browser, not three, so it appears as if the extra whitespace is collapsed either when used to generate the HTML or when the HTML is rendered.

@asvetlov asvetlov merged commit f1f0c58 into python:3.8 Jan 12, 2020
6 checks passed
6 checks passed
Docs
Details
Azure Pipelines PR #20200112.4 succeeded
Details
bedevere/issue-number Issue report skipped
bedevere/maintenance-branch-pr Valid maintenance branch PR title.
bedevere/news "skip news" label found
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@asvetlov

This comment has been minimized.

Copy link
Contributor

asvetlov commented Jan 12, 2020

Thanks!

@toonarmycaptain toonarmycaptain deleted the toonarmycaptain:patch-3 branch Jan 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.