Skip to content

Improve readability of formataddr docstring#20963

Merged
miss-islington merged 2 commits into
python:masterfrom
jugmac00:improve_readability
Jun 19, 2020
Merged

Improve readability of formataddr docstring#20963
miss-islington merged 2 commits into
python:masterfrom
jugmac00:improve_readability

Conversation

@jugmac00
Copy link
Copy Markdown
Contributor

@jugmac00 jugmac00 commented Jun 18, 2020 •
edited by miss-islington
Loading

For me as a non native English speaker, the sentence with its embedded clause was very hard to understand.

modified: Lib/email/utils.py

Automerge-Triggered-By: @csabella

@jugmac00
Improve readability of formataddr docstring
f27addb 
modified:   Lib/email/utils.py
@jugmac00 jugmac00 requested a review from a team as a code owner June 18, 2020 13:50
eamanu
eamanu reviewed Jun 19, 2020
View reviewed changes
Comment thread Lib/email/utils.py Outdated
returned unmodified.

Optional charset if given is the character set that is used to encode
Optional charset, if given, is the character set that is used to encode
Copy link
Copy Markdown
Contributor

@eamanu eamanu Jun 19, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I agree to this change. IMHO for no natives is more natural read this, but I don't know if this change is normal for a native English speaker.

But, I think that we should remove the if given sentences, because, when the docstring say "Optional charset" that reference that charset could be given or not. Optional params.

Copy link
Copy Markdown
Contributor Author

@jugmac00 jugmac00 Jun 19, 2020

Choose a reason for hiding this comment

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

Thanks for your reply. I am happy about any change which makes the docstring more readable.

And indeed, the sentence is much nicer without the if given clause.

I applied your suggestion with a follow up commit.

I noticed this if given pattern is used more often in this module. Should the other places also be updated?

@jugmac00
Remove redundant if given clause
2ccf6e9 
This makes the docstring easier to comprehend.

modified:   Lib/email/utils.py
csabella
csabella approved these changes Jun 19, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@csabella csabella left a comment

Choose a reason for hiding this comment

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

LGTM

@csabella
Copy link
Copy Markdown
Contributor

csabella commented Jun 19, 2020

@jugmac00, thank you for the suggestion and @eamanu, thank you for the review. I agree that the if given phrase is redundant with optional.

@miss-islington
Copy link
Copy Markdown
Contributor

miss-islington commented Jun 19, 2020

Sorry, I can't merge this PR. Reason: Base branch was modified. Review and try the merge again..

@miss-islington miss-islington merged commit 66a65ba into python:master Jun 19, 2020
@miss-islington
Copy link
Copy Markdown
Contributor

miss-islington commented Jun 19, 2020

@jugmac00: Status check is done, and it's a success ✅ .

1 similar comment
@miss-islington
Copy link
Copy Markdown
Contributor

miss-islington commented Jun 19, 2020

@jugmac00: Status check is done, and it's a success ✅ .

@miss-islington
Copy link
Copy Markdown
Contributor

miss-islington commented Jun 19, 2020

Sorry, I can't merge this PR. Reason: Pull Request is not mergeable.

1 similar comment
@miss-islington
Copy link
Copy Markdown
Contributor

miss-islington commented Jun 19, 2020

Sorry, I can't merge this PR. Reason: Pull Request is not mergeable.

@miss-islington
Copy link
Copy Markdown
Contributor

miss-islington commented Jun 19, 2020

Thanks @jugmac00 for the PR 🌮🎉.. I'm working now to backport this PR to: 3.8, 3.9.
🐍🍒⛏🤖

@miss-islington
Copy link
Copy Markdown
Contributor

miss-islington commented Jun 19, 2020

Sorry @jugmac00, I had trouble checking out the 3.9 backport branch.
Please backport using cherry_picker on command line.
cherry_picker 66a65ba43cb3e68a43e32469c988dd7a6cff049c 3.9

@miss-islington miss-islington self-assigned this Jun 19, 2020
@bedevere-bot
Copy link
Copy Markdown

bedevere-bot commented Jun 19, 2020

GH-20977 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 Jun 19, 2020
@jugmac00 @miss-islington
Improve readability of formataddr docstring (pythonGH-20963)
401d223 
For me as a non native English speaker, the sentence with its embedded clause was very hard to understand.

modified:   Lib/email/utils.py

Automerge-Triggered-By: @csabella
(cherry picked from commit 66a65ba)

Co-authored-by: Jürgen Gmach <juergen.gmach@googlemail.com>
@miss-islington
Copy link
Copy Markdown
Contributor

miss-islington commented Jun 19, 2020

Thanks @jugmac00 for the PR 🌮🎉.. I'm working now to backport this PR to: 3.9.
🐍🍒⛏🤖

@bedevere-bot
Copy link
Copy Markdown

bedevere-bot commented Jun 19, 2020

GH-20978 is a backport of this pull request to the 3.9 branch.

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jun 19, 2020
@jugmac00 @miss-islington
Improve readability of formataddr docstring (pythonGH-20963)
0bd3d8c 
For me as a non native English speaker, the sentence with its embedded clause was very hard to understand.

modified:   Lib/email/utils.py

Automerge-Triggered-By: @csabella
(cherry picked from commit 66a65ba)

Co-authored-by: Jürgen Gmach <juergen.gmach@googlemail.com>
miss-islington added a commit that referenced this pull request Jun 19, 2020
@miss-islington @jugmac00
Improve readability of formataddr docstring (GH-20963)
4dd10ed 
For me as a non native English speaker, the sentence with its embedded clause was very hard to understand.

modified:   Lib/email/utils.py

Automerge-Triggered-By: @csabella
(cherry picked from commit 66a65ba)

Co-authored-by: Jürgen Gmach <juergen.gmach@googlemail.com>
miss-islington added a commit that referenced this pull request Jun 19, 2020
@miss-islington @jugmac00
Improve readability of formataddr docstring (GH-20963)
83e54de 
For me as a non native English speaker, the sentence with its embedded clause was very hard to understand.

modified:   Lib/email/utils.py

Automerge-Triggered-By: @csabella
(cherry picked from commit 66a65ba)

Co-authored-by: Jürgen Gmach <juergen.gmach@googlemail.com>
fasih pushed a commit to fasih/cpython that referenced this pull request Jun 29, 2020
@jugmac00 @fasihahmad
Improve readability of formataddr docstring (pythonGH-20963)
c4fb656 
For me as a non native English speaker, the sentence with its embedded clause was very hard to understand.

modified:   Lib/email/utils.py

Automerge-Triggered-By: @csabella
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@csabella csabella csabella approved these changes

+1 more reviewer

@eamanu eamanu eamanu left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@miss-islington miss-islington

Labels

skip issue skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@jugmac00 @csabella @miss-islington @bedevere-bot @eamanu @the-knights-who-say-ni