-
-
Notifications
You must be signed in to change notification settings - Fork 30k
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
bpo-31507 Add docstring to parseaddr function in email.utils.parseaddr #3647
Conversation
Adds docstring to email.utils.parseaddr
Hello, and thanks for your contribution! I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA). Unfortunately we couldn't find an account corresponding to your GitHub username on bugs.python.org (b.p.o) to verify you have signed the CLA (this might be simply due to a missing "GitHub Name" entry in your b.p.o account settings). This is necessary for legal reasons before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue. Thanks again to your contribution and we look forward to looking at it! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on this.
Lib/email/utils.py
Outdated
– into its constituent realname and email address parts. | ||
Returns a tuple of that information, unless the parse fails, | ||
in which case a 2-tuple of ('', '') is returned. | ||
""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The style we try to follow for docstrings is a single initial sentence that tries to summarize the most important parts of the API, followed by a blank line, followed by a more extensive description of anything not covered in the initial sentence. The entire thing should be in imperative mode (we are not consistent about that in the docstrings and docs, but that is my understanding of our desired style).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My bad. I just copy-pasted the entire thing from the python docs. I'll try to make it more readable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your change doesn't seem to have done anything other than make the line lengths longer than our 79 char standard. There is a reason why we don't use autodoc: docstrings and the main docs usually have somewhat different wording (and the docstrings are usually shorter, although in this case they may not be).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I re-read your instructions carefully and I think I finally got it right. I'll make sure I read the python developer's guide before I further contribute to this language.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure how much of this docstring detail is in the devguide. It might be mentioned in PEP 8, or maybe there's a separate informational PEP about docstrings, I forget.
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
I didn't expect the Spanish Inquisition! I hope the changes are satisfactory. I tried to follow your instructions for this one but the description of email.utils.parseaddr from the docs doesn't quite follow the guidelines for docstrings because of it's unique nature. If you want to, I can change the description and get it from a third-party |
Nobody expects the Spanish Inquisition! @bitdancer: please review the changes made to this pull request. |
Lib/email/utils.py
Outdated
@@ -215,6 +215,14 @@ def parsedate_to_datetime(data): | |||
|
|||
|
|||
def parseaddr(addr): | |||
""" | |||
Parse address into its constituent | |||
realname and email address parts |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be a single line ending with a period. Instead of 'address', use the variable name from the prototype: 'addr'.
Lib/email/utils.py
Outdated
|
||
Returns a tuple of realname and email address, | ||
unless the parse fails, | ||
in which case a 2-tuple of ('', '') is returned. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wrap this into a paragraph with a max width of 79 characters. Change 'Returns' to 'Return' (imperative voice), and likewise "in which case" becomes "in which case return" and drop 'is returned'.
@@ -0,0 +1 @@ | |||
Adds docstring to email.utils.parseaddr |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove this. We don't need doc entries for news items (I'll add the skip news label), and in any case IDLE is the wrong section :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I figured that this didn't a news entry. But I had no idea how to add the skip news label and I wasn't sure where else to add an entry other than IDLE(a quick file search showed such entries in IDLE). You went above and beyond to help me with this, thank you 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No problem. This is part of nurturing new contributors :)
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
Lib/email/utils.py
Outdated
Parse addr into its constituent realname and email address parts. | ||
|
||
Return a tuple of realname and email address, unless the parse fails, in which | ||
case return a 2-tuple of ('', '') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are missing a trailing period here, otherwise this looks good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah looks like I need grammar lessons again
Hmm. I guess we should wait for your CLA signing to come through (you did sign it, right?) I doubt it really applies to this change, which is a rewrite of the existing docs, but better safe than sorry :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm. Travis is saying you have traling whitespace. I thought github showed that, but apparently not.
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
I fixed the whitespace at the end of the code (my bad, never noticed it). And I did sign the CLA yesterday. Do I also have to make changes to the 2.7 and 3.6 branch too? Edit: looks like I was wrong about the whitespace. I'll fix it soon |
@bitdancer I think everything is okay now. |
Thanks @AstroCat84 for the PR, and @bitdancer for merging it 🌮🎉.. I'm working now to backport this PR to: 2.7, 3.6. |
GH-3733 is a backport of this pull request to the 3.6 branch. |
GH-3735 is a backport of this pull request to the 2.7 branch. |
Adds docstring to email.utils.parseaddr
Closes Issue31507 (hopefully) from python bug tracker
I'm new to this so please forgive me if I make mistakes
https://bugs.python.org/issue31507