bpo-31507 Add docstring to parseaddr function in email.utils.parseaddr by AstroCat84 · Pull Request #3647 · python/cpython

AstroCat84 · 2017-09-18T14:53:19Z

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

the-knights-who-say-ni · 2017-09-18T14:53:22Z

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!

bitdancer

Thanks for working on this.

bitdancer · 2017-09-18T15:35:47Z

+    – 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.
+    """


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).

My bad. I just copy-pasted the entire thing from the python docs. I'll try to make it more readable

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).

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.

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.

bedevere-bot · 2017-09-18T15:36:21Z

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.

AstroCat84 · 2017-09-18T16:33:16Z

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

bedevere-bot · 2017-09-18T16:33:18Z

Nobody expects the Spanish Inquisition!

@bitdancer: please review the changes made to this pull request.

bitdancer · 2017-09-19T13:23:44Z

 def parseaddr(addr):
+    """
+    Parse address into its constituent 
+    realname and email address parts


This should be a single line ending with a period. Instead of 'address', use the variable name from the prototype: 'addr'.

bitdancer · 2017-09-19T13:26:36Z

+
+    Returns a tuple of realname and email address,
+    unless the parse fails,
+    in which case a 2-tuple of ('', '') is returned.


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'.

bitdancer · 2017-09-19T13:27:36Z

@@ -0,0 +1 @@
+Adds docstring to email.utils.parseaddr


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 :)

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 👍

No problem. This is part of nurturing new contributors :)

bedevere-bot · 2017-09-19T13:28:11Z

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.

bitdancer · 2017-09-19T15:16:17Z

+    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 ('', '')


You are missing a trailing period here, otherwise this looks good.

Ah looks like I need grammar lessons again

bitdancer · 2017-09-19T15:57:36Z

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 :)

bitdancer

Hmm. Travis is saying you have traling whitespace. I thought github showed that, but apparently not.

bedevere-bot · 2017-09-19T15:58:36Z

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.

AstroCat84 · 2017-09-19T16:14:20Z

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

AstroCat84 · 2017-09-19T18:59:41Z

@bitdancer I think everything is okay now.

miss-islington · 2017-09-19T19:10:52Z

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

bedevere-bot · 2017-09-24T19:42:28Z

GH-3733 is a backport of this pull request to the 3.6 branch.

bedevere-bot · 2017-09-24T19:57:14Z

GH-3735 is a backport of this pull request to the 2.7 branch.

GH-3647) (GH-3733) (cherry picked from commit 9e7b9b2)

…dr (GH-3647) (GH-3735) (cherry picked from commit 9e7b9b2)

Update utils.py

ba7394d

Adds docstring to email.utils.parseaddr

AstroCat84 requested a review from a team as a code owner September 18, 2017 14:53

bedevere-bot added the awaiting review label Sep 18, 2017

the-knights-who-say-ni added the CLA not signed label Sep 18, 2017

AstroCat84 changed the title ~~Update utils.py~~ Update utils.py #31507 Sep 18, 2017

AstroCat84 changed the title ~~Update utils.py #31507~~ bpo-31507 Update utils.py Sep 18, 2017

Update utils.py

cb65777

bitdancer requested changes Sep 18, 2017

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting review labels Sep 18, 2017

Update utils.py

c60a640

the-knights-who-say-ni added CLA signed and removed CLA not signed labels Sep 18, 2017

Create 2017-09-18.bpo-31507.dgs67n.rst

3aac0e8

bedevere-bot added awaiting change review and removed awaiting changes labels Sep 18, 2017

Update utils.py

d6207a6

bitdancer requested changes Sep 19, 2017

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting change review labels Sep 19, 2017

bitdancer added the skip news label Sep 19, 2017

AstroCat84 added 2 commits September 19, 2017 20:25

Delete 2017-09-18.bpo-31507.dgs67n.rst

59e50cd

Update utils.py

15273ff

bitdancer requested changes Sep 19, 2017

View reviewed changes

Update utils.py

87bb480

bitdancer added needs backport to 2.7 labels Sep 19, 2017

bitdancer requested changes Sep 19, 2017

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting merge labels Sep 19, 2017

Update utils.py

a92507a

AstroCat84 added 5 commits September 19, 2017 22:13

Update utils.py

bd86a61

Update utils.py

36fbe50

Update utils.py

55f4a24

Update utils.py

1d80458

Update utils.py

57be3f4

AstroCat84 changed the title ~~bpo-31507 Update utils.py~~ bpo-31507 Add docstring to parseaddr function in email.utils.parseaddr Sep 19, 2017

bitdancer approved these changes Sep 19, 2017

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting changes labels Sep 19, 2017

bitdancer merged commit 9e7b9b2 into python:master Sep 19, 2017

AstroCat84 deleted the patch-1 branch September 19, 2017 19:14

bedevere-bot removed the needs backport to 3.6 label Sep 24, 2017

bedevere-bot removed the needs backport to 2.7 label Sep 24, 2017

Mariatta pushed a commit that referenced this pull request Oct 7, 2017

bpo-31507 Add docstring to parseaddr function in email.utils.parseaddr (

93c0885

GH-3647) (GH-3733) (cherry picked from commit 9e7b9b2)

Mariatta pushed a commit that referenced this pull request Oct 7, 2017

bpo-31507: Add docstring to parseaddr function in email.utils.parsead…

c829bbf

…dr (GH-3647) (GH-3735) (cherry picked from commit 9e7b9b2)

Mariatta removed the awaiting merge label Oct 8, 2017

Uh oh!

Uh oh!

Conversation

AstroCat84 commented Sep 18, 2017 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

the-knights-who-say-ni commented Sep 18, 2017

Uh oh!

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

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Sep 18, 2017

Uh oh!

AstroCat84 commented Sep 18, 2017

Uh oh!

bedevere-bot commented Sep 18, 2017

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!

AstroCat84 Sep 19, 2017 • 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!

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Sep 19, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bitdancer commented Sep 19, 2017

Uh oh!

bitdancer left a comment

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Sep 19, 2017

Uh oh!

AstroCat84 commented Sep 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AstroCat84 commented Sep 19, 2017

Uh oh!

miss-islington commented Sep 19, 2017

Uh oh!

bedevere-bot commented Sep 24, 2017

Uh oh!

bedevere-bot commented Sep 24, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

AstroCat84 commented Sep 18, 2017 •

edited by bedevere-bot

Loading

AstroCat84 Sep 19, 2017 •

edited

Loading

AstroCat84 commented Sep 19, 2017 •

edited

Loading