bpo-32297: Few misspellings found in Python source code comments. #4803

mehanig · 2017-12-11T23:56:19Z

I've extracted all the comments from *.py files and re-read them manually with the help of spelling and grammar tools.
Most of the changes are a misspelling or something like incorrect editing. I wasn't expected to touch intentionally any other files except *.py, but while searching through codebase found few mistakes also.

And, few files are antique (10+ years), so I feel very nervous touching that part of the Python history.

https://bugs.python.org/issue32297

mehanig · 2017-12-12T00:00:41Z

Lib/test/test_importlib/source/test_file_loader.py

I decided to change function name for the sake of correct spelling. It shouldn't affect any other code and looks like tests aren't failing.

I think this is okay since it won't affect anything else, and test can run as it is.

Mariatta · 2017-12-12T00:20:10Z

Hmm, the fixes here are for internal comments, not docstrings or documentation. I'm thinking these should be left as is, unless we are actually fixing those parts of the code.

terryjreedy

Mike, thank you for adding Python to the list of projects you contribute to. I presume you carefully checked the changes yourself. I double-checked and all look correct. Although not public, they make the code nicer to read.

I am the maintainer of idlelib, which is a special case, in that I even backport changes like this. So I will make the 3 changes in a separate patch and then revert them on this PR. IE, I will do the change I am requesting. Edit: Done #4807, #4809

I believe Mariatta's concern is about code churn and blame history. I will let everyone else decide what to do with non-idlelib changes.

Though each change is trivial, there are enough that I would have preferred that you started with a tracker issue. But I don't care now.

Test name changes should not affect anything else. If the test were my test (in idle_test), I would accept the name change, but it is not, so I leave it to the import people.

mehanig · 2017-12-12T12:31:43Z

Thank you, Terry. Yes, I agree that it's might be good to open an Issue and discuss whether or not we should merge this request (partially or entirely).
I didn't know about idlelib special cases, thank you a lot for helping with it and backporting changes.

Mariatta, you are 100% correct, most of the changes are internal code comments, except one docstring (https://github.com/python/cpython/pull/4803/files#diff-be61682fc3333d3bd8fc466a4e4dbdcfR1261). And I completely agree with you that it might be useful, in some cases, to not touch git blame history.

I think, at this moment, it's up for maintainers to decline or approve this changes, and I am open to exclude any of them from this request.

brettcannon · 2017-12-12T20:28:38Z

Can you open an issue @mehanig on bugs.python.org and edit the title to reference it so we can decide if we want to accept your PR?

mehanig · 2017-12-13T00:14:35Z

Yes, done. Sorry for my late reply.

vstinner · 2017-12-13T00:20:31Z

@Mariatta: "Hmm, the fixes here are for internal comments, not docstrings or documentation. I'm thinking these should be left as is, unless we are actually fixing those parts of the code."

@terryjreedy: "I believe Mariatta's concern is about code churn and blame history. I will let everyone else decide what to do with non-idlelib changes."

IMHO it's ok to fix typos :-) But I didn't review the change itself. I asked @csabella and @CuriousLearner to take a look.

Mariatta · 2017-12-13T00:27:07Z

Yeah well, it makes me uneasy when a PR touched 41 different files.
I suspect there will be conflict when backporting...
If other core devs think it's worth fixing, I really won't object.
I did not explicitly approve/reject the PR. Simply raising a concern :)

vstinner · 2017-12-13T00:36:03Z

I suspect there will be conflict when backporting...

Ah right, we have to backport this PR to Python 3.6 if we choose to merge it, to reduce conflicts when backporting fixes into Python 3.6.

I propose to not backport it to Python 2.7, since Python 2.7 is dying and minor fixes are no more backported to 2.7.

CuriousLearner

I think for the spell fixes in comment and test cases are okay, since it won't affect other parts.

On the other hand, I agree that it may cause some issue in back-porting it to 3.6.

Apart from that, I have just minor comments on things. If this change is merged in, it would change the git blame for those lines, so it makes sense to do these changes too.

CuriousLearner · 2017-12-13T05:16:17Z

Lib/test/test_logging.py

If you're fixing this sentence. I would suggest to also leave a space between # and the first word for the sentence.

Yes, I agree. I even fixed all the other places in this file. Otherwise, it might look very inconsistent. I'm talking about this last commit 53592fc, but it might be overhead and I'm open to revert it.

CuriousLearner · 2017-12-13T05:18:38Z

Lib/test/test_math.py

For the comments on the same line as code, please leave 2 spaces before starting the comment.

If we choose to change the git blame for this line, then it makes sense to fix as much as we can :)

Good point, done in e2321c9

csabella · 2017-12-13T14:01:57Z

I agree with @CuriousLearner 's suggestions and the other changes LGTM.

If it helps, 26 of the 41 file changes are for test files, so I don't know if the blame history is as important on those. Some of the changes to code files are ones that have been modified a few times this year, like asyncio, http, os, xml, and faulthandler.c. Maybe the maintainers of those should make individual decisions for blame, while the rest of the module changes might not be happen often enough to worry about the history.

asvetlov · 2017-12-13T14:08:51Z

#4839 for asyncio typos

mehanig · 2017-12-13T15:58:06Z

@asvetlov Thank you. Is it correct, that I need now to exclude from this request the changes you picked?

asvetlov · 2017-12-13T15:59:21Z

Just merge master branch from upstream

CuriousLearner

Changes you did till
e2321c9, LGTM.

I don't think changes done in 53592fc are necessary.

Since we were fixing typos in previous comments, it made sense to also correctly format the comments.

The module owner can take a decision if the diff in 53592fc is really needed :)

Thanks for your work :)

mehanig · 2017-12-13T16:04:10Z

@CuriousLearner Good! Yes, I'm also not sure about the last one :)

brettcannon · 2017-12-13T18:45:12Z

FYI I approve of the importlib test changes.

JelleZijlstra · 2017-12-14T07:23:22Z

Lib/idlelib/autocomplete_w.py

Here it looks like the PR is introducing a typo. Did something go wrong with the separate idlelib change?

Thank you, my bad, incorrect rebase. Since current master already merged that changes separately, I now excluded idlelib revert-patches from this request.

miss-islington · 2017-12-14T11:04:55Z

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

miss-islington · 2017-12-14T11:05:00Z

Sorry, @mehanig and @asvetlov, I could not cleanly backport this to 3.6 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 53f7a7c2814fbfd8a29200926601a32fa48bacb3 3.6

asvetlov · 2017-12-14T11:05:03Z

Thanks!

vstinner · 2017-12-14T11:08:05Z

@mehanig: Can you please backport your fix to Python 3.6? The backport will prevent conflicts when other changes will have to be backported.

asvetlov · 2017-12-14T11:08:20Z

Backport to 3.6 has merge conflicts, let's not touch the branch.

vstinner · 2017-12-14T11:13:42Z

Backport to 3.6 has merge conflicts, let's not touch the branch.

Please review previous comments starting near:
#4803 (comment)

This PR must be backported to Python 3.6. @mehanig (or someone else) can easily fix conflicts.

…s. (pythonGH-4803) * Fix multiple typos in code comments * Add spacing in comments (test_logging.py, test_math.py) * Fix spaces at the beginning of comments in test_logging.py. (cherry picked from commit 53f7a7c)

asvetlov · 2017-12-14T11:15:36Z

Ok, I did it: #4864

…s. (pythonGH-4803) * Fix multiple typos in code comments * Add spacing in comments (test_logging.py, test_math.py) * Fix spaces at the beginning of comments in test_logging.py. (cherry picked from commit 53f7a7c)

mehanig · 2017-12-14T12:09:27Z

@asvetlov Thank you so much for your help!

…4803) (#4864) * [3.6] bpo-32297: Few misspellings found in Python source code comments. (GH-4803) * Fix multiple typos in code comments * Add spacing in comments (test_logging.py, test_math.py) * Fix spaces at the beginning of comments in test_logging.py. (cherry picked from commit 53f7a7c)

mehanig requested review from a team, 1st1, ericvsmith, ncoghlan, rhettinger and terryjreedy as code owners December 11, 2017 23:56

mehanig requested a review from a team December 11, 2017 23:56

the-knights-who-say-ni added the CLA signed label Dec 11, 2017

bedevere-bot added the awaiting review label Dec 11, 2017

mehanig commented Dec 12, 2017

View reviewed changes

ericvsmith added skip issue skip news labels Dec 12, 2017

terryjreedy requested changes Dec 12, 2017

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting review labels Dec 12, 2017

terryjreedy approved these changes Dec 12, 2017

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting changes labels Dec 12, 2017

python deleted a comment from bedevere-bot Dec 12, 2017

brettcannon removed the skip issue label Dec 12, 2017

mehanig changed the title ~~Fix multiple typos in code comments.~~ bpo-32297: Few misspellings found in Python source code comments. Dec 13, 2017

CuriousLearner reviewed Dec 13, 2017

View reviewed changes

Fix multiple typos in code comments

1c4b950

mehanig force-pushed the fix-multiple-comments-typos branch from 53592fc to af35990 Compare December 13, 2017 16:22

JelleZijlstra reviewed Dec 14, 2017

View reviewed changes

mehanig added 2 commits December 14, 2017 12:07

Add spacing in comments (test_logging.py, test_math.py)

14beea5

Fix spaces at the beginning of comments in test_logging.py

b58b67b

mehanig force-pushed the fix-multiple-comments-typos branch from af35990 to b58b67b Compare December 14, 2017 09:08

rhettinger approved these changes Dec 14, 2017

View reviewed changes

asvetlov approved these changes Dec 14, 2017

View reviewed changes

asvetlov added the needs backport to 3.6 label Dec 14, 2017

asvetlov merged commit 53f7a7c into python:master Dec 14, 2017

bedevere-bot removed the awaiting merge label Dec 14, 2017

asvetlov removed the needs backport to 3.6 label Dec 14, 2017

Uh oh!

bpo-32297: Few misspellings found in Python source code comments. #4803

bpo-32297: Few misspellings found in Python source code comments. #4803

Uh oh!

Conversation

mehanig commented Dec 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Mariatta commented Dec 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

terryjreedy 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!

mehanig commented Dec 12, 2017

Uh oh!

brettcannon commented Dec 12, 2017

Uh oh!

mehanig commented Dec 13, 2017

Uh oh!

vstinner commented Dec 13, 2017

Uh oh!

Mariatta commented Dec 13, 2017

Uh oh!

vstinner commented Dec 13, 2017

Uh oh!

CuriousLearner left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

csabella commented Dec 13, 2017

Uh oh!

asvetlov commented Dec 13, 2017

Uh oh!

mehanig commented Dec 13, 2017

Uh oh!

asvetlov commented Dec 13, 2017

Uh oh!

CuriousLearner left a comment

Choose a reason for hiding this comment

Uh oh!

mehanig commented Dec 13, 2017

Uh oh!

brettcannon commented Dec 13, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

miss-islington commented Dec 14, 2017

Uh oh!

miss-islington commented Dec 14, 2017

Uh oh!

asvetlov commented Dec 14, 2017

Uh oh!

vstinner commented Dec 14, 2017

Uh oh!

asvetlov commented Dec 14, 2017

Uh oh!

vstinner commented Dec 14, 2017

Uh oh!

asvetlov commented Dec 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mehanig commented Dec 14, 2017

Uh oh!

Reviewers

Assignees

mehanig commented Dec 11, 2017 •

edited

Loading

Mariatta commented Dec 12, 2017 •

edited

Loading

terryjreedy left a comment •

edited

Loading

asvetlov commented Dec 14, 2017 •

edited

Loading