bpo-31507 Add docstring to parseaddr function in email.utils.parseaddr by AstroCat84 · Pull Request #3647 · python/cpython (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation28 Commits15 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
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
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!
Update utils.py Update utils.py #31507
Update utils.py #31507 bpo-31507 Update utils.py
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.
| – 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. |
| """ |
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).
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
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).
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.
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 will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.
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
| @@ -215,6 +215,14 @@ def parsedate_to_datetime(data): |
|---|
| def parseaddr(addr): |
| """ |
| Parse address into its constituent |
| realname and email address parts |
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'.
| Returns a tuple of realname and email address, |
| unless the parse fails, |
| in which case a 2-tuple of ('', '') is returned. |
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 |
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 :)
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 👍
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 I didn't expect the Spanish Inquisition!. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.
| 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 ('', '') |
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.
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 :)
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 didn't expect the Spanish Inquisition!. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.
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
bpo-31507 Update utils.py bpo-31507 Add docstring to parseaddr function in email.utils.parseaddr
Thanks @AstroCat84 for the PR, and @bitdancer for merging it 🌮🎉.. I'm working now to backport this PR to: 2.7, 3.6.
🐍🍒⛏🤖
Mariatta pushed a commit that referenced this pull request
Mariatta pushed a commit that referenced this pull request