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

Fix typos in docs, comments and test assert messages #14872

Merged
merged 7 commits into from
Jul 21, 2019
Merged

Fix typos in docs, comments and test assert messages #14872

merged 7 commits into from
Jul 21, 2019

Conversation

minho42
Copy link
Contributor

@minho42 minho42 commented Jul 21, 2019

No description provided.

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.

@minho42 Thanks for the contribution! I've seen quite a number of PRs which fix typos, but I've yet to see one involving 22 different files at once. Nicely done.

I approve of all of the typo fixes, but I noticed a few which could use some minor improvements.

Doc/library/ctypes.rst Show resolved Hide resolved
Lib/test/support/__init__.py Outdated Show resolved Hide resolved
Lib/test/test_dataclasses.py Show resolved Hide resolved
Lib/test/test_dataclasses.py Show resolved Hide resolved
Lib/test/test_dataclasses.py Show resolved Hide resolved
Lib/test/test_importlib/test_main.py Outdated Show resolved Hide resolved
Copy link
Member

@terryjreedy terryjreedy left a comment

Choose a reason for hiding this comment

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

I have verified that all but 1 of the corrections is correct and that that 1 is corrected in the comment. That should be enough review of the corrections themselves. I approve of this given that one correction. I consider the one wording change I approved of to be optional.

'minho': I am willing to merge this, but I first need to know if you intend to fix up the merge conflicts I expect if we try to backport to 3.8, or 3.7. I expect that at least one misspelling is new in master.

minho42 and others added 3 commits July 21, 2019 18:11
Co-Authored-By: Kyle Stanley <aeros167@gmail.com>
Co-Authored-By: Kyle Stanley <aeros167@gmail.com>
Co-Authored-By: Kyle Stanley <aeros167@gmail.com>
@minho42
Copy link
Contributor Author

minho42 commented Jul 21, 2019

@terryjreedy
Are you able to merge this?
Does that mean you are going to fix the merge conflicts?

I'd like to resolve the merge conflicts myself but I won't be able to use my laptop for the next few days.

@minho42
Copy link
Contributor Author

minho42 commented Jul 21, 2019

@aeros167
Thanks for the suggestions.

@aeros
Copy link
Contributor

aeros commented Jul 21, 2019

@minho42

Are you able to merge this?
Does that mean you are going to fix the merge conflicts?

Unless I'm misunderstanding what terry said, the merge conflicts only need to be resolved if he adds the backport labels so that these changes are included in the 3.8 and 3.7 docs. If it's only merged into 3.9 without a backport, the merge conflicts shouldn't be an issue.

If all of the deployment tests are passing and there's no indication otherwise, this should be able to get auto-merged into 3.9 without issues.

@aeros
Copy link
Contributor

aeros commented Jul 21, 2019

@minho42 Undo the following commit:
d91fb72

(based on the review from eric)

@minho42
Copy link
Contributor Author

minho42 commented Jul 21, 2019

@aeros167
Then I'm happy to let this merged into 3.9 without backporting!

@minho42
Copy link
Contributor Author

minho42 commented Jul 21, 2019

@aeros167
Changed back.

Copy link
Member

@terryjreedy terryjreedy left a comment

Choose a reason for hiding this comment

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

Two doc build tests failed with what seems like a false positive

Warning, treated as error:
[distutils/examples:267] "`" found in "This is the description of the ``foobar`` package."

This makes no sense to me because this PR does not touch this doc and because I think the backticks are normal and not an error or even 'suspcious'.

I moved the idlelib fix to a separate PR, #14879, to make sure it gets immediately backported. I keep help.py synchronized across active versions. This change will trigger a test there and a retest here.

@terryjreedy terryjreedy reopened this Jul 21, 2019
@terryjreedy terryjreedy merged commit 96e12d5 into python:master Jul 21, 2019
@bedevere-bot
Copy link

@terryjreedy: Please replace # with GH- in the commit message next time. Thanks!

@miss-islington
Copy link
Contributor

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

@miss-islington
Copy link
Contributor

Sorry, @minho42 and @terryjreedy, I could not cleanly backport this to 3.8 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 96e12d5f4f3c5a20986566038ee763dff3c228a1 3.8

@terryjreedy
Copy link
Member

Thank you for the corrections. I was correct that backport would need fixup. I am done with this unless one of you wants to do them.

@aeros
Copy link
Contributor

aeros commented Jul 21, 2019

@terryjreedy I wouldn't mind trying to help work through the merge conflicts, but I don't think there's way for me to directly edit the PR.

@terryjreedy
Copy link
Member

Right, you prepare a new pr, much as the bot does, using the cherry_picker app on pypi, with the command line given above, except that you continue where the bot quit. To install for recent python,
python -m pip install cherry_picker
There may be some doc on the site. Or
python -m cherry_picker -h
for help. (I use python - m because cherry_picker -h may or may not work, depending on your install.)
It should be educational, and more requiring of attention to detail than really hard. This should be a good pr to practice with.

You can use git cherry-pick directly, instead, but cherry_picker will do the routine parts, including pushing to github and making a PR.

@minho42
Copy link
Contributor Author

minho42 commented Jul 21, 2019

Thanks @aeros167 @terryjreedy

@aeros
Copy link
Contributor

aeros commented Jul 22, 2019

@terryjreedy Sounds good, I'll give it a try then. I'm interested in the new GitHub triager role, so it would be good to learn how to manually (with cherry_picker) backport and reserve merge conflicts.

When opening the new PR, I should name it something using a similar format to the PR you did, but with 3.8 in brackets?:
[3.8] Fix typos found by Min Ho Kim

Also, I'm assuming that I should specifically direct the PR at the branch python:3.8 instead of the usual python:master. Apologies if any of this is fairly obvious, I'm not as familiar with the manual back porting process. I'll make a comment in here linking to it when it's ready.

Edit: The naming and branch to point at was assuming the manual process with git cherry-pick in case the automatic process has any issues. I'll try to do it w/ cherry_picker first.

aeros pushed a commit to aeros/cpython that referenced this pull request Jul 22, 2019
…14872).

(cherry picked from commit 96e12d5)

Co-authored-by: Min ho Kim <minho42@gmail.com>
@bedevere-bot
Copy link

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

@aeros
Copy link
Contributor

aeros commented Jul 22, 2019

I was able to resolve the conflicts and create the backport with cherry_picker. It looks like all of the tests are passing, let me know if there's any issues: #14900. Should I attempt to do to the same for 3.7 as well?

terryjreedy pushed a commit that referenced this pull request Jul 22, 2019
#14900)

(cherry picked from commit 96e12d5)

Co-authored-by: Min ho Kim <minho42@gmail.com>
@terryjreedy
Copy link
Member

In response to cherry_picker 96e12d5f4f3c5a20986566038ee763dff3c228a1 3.8, cherry_picker checks out permanent branch 3.8, creates (checkout -b) a branch based on that (of whatever name, not important), and then applies the indicated commit diff. If there are no merge conflicts, it proceeds. Otherwise it stops and tells you to fix them. It copies the original title and adds [3.8] (in this case). When you tell it to continue, it commits with the original message and adds the boilerplate. You saw what you got.

To do 3.7, change 3.8 to 3.7 and leave the commit-to-master hash as is. The conventions here is that all backports are from the original commit to master. A git expert (not me ;-) could fake it by manually applying the (in this case) reduced 3.8 diff, fixing any additional conflicts, and then adding the comment

Original PR: #14900 
(cherry picked from commit 96e12d5)

as long as the result was the same as if that were done.

These occasional typo fixes are about the only PRs with such increasing conflicts.

aeros pushed a commit to aeros/cpython that referenced this pull request Jul 22, 2019
…14872).

(cherry picked from commit 96e12d5)

Co-authored-by: Min ho Kim <minho42@gmail.com>
@aeros
Copy link
Contributor

aeros commented Jul 22, 2019

Created backport for 3.7: GH-14901. There were significantly more conflicts with this one, and several of the sections that changes were made to did not exist in 3.7.

terryjreedy pushed a commit that referenced this pull request Jul 22, 2019
#14901)

(cherry picked from commit 96e12d5)

Co-authored-by: Min ho Kim <minho42@gmail.com>
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants