bpo-30181: parse docstring using pydoc #1505

brianmay · 2017-05-09T02:02:57Z

Rebased and copied Ben Finney's changes from http://bugs.python.org/issue30181

I hope master is the correct branch to merge this into.

https://bugs.python.org/issue30181

PEP 257 specifies that tools which parse docstrings should strip surrounding blank lines *before* determining what is the first line <URL:https://www.python.org/dev/peps/pep-0257/#handling-docstring-indentation>.

the-knights-who-say-ni · 2017-05-09T02:03:00Z

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

brianmay · 2017-05-09T02:08:08Z

Apparently trivial changes don't require a CLA. Does this count as a trivial change? In any case, the changes are from Ben Finney, not myself, so I suspect it isn't going to help if I sign the CLA.

brianmay · 2017-05-09T04:24:20Z

The two tests that fail don't appear to be valid PEP 257 docstring to me. So I think maybe the tests need to be modified.

serhiy-storchaka · 2017-05-09T10:53:35Z

Lib/unittest/case.py

+    """
+    Return the test case short description from a docstring.
+
+    Ths docstring is parsed by the standard `pydoc.splitdoc` function


This is an implementation detail. Both short_description_from_docstring and pydoc.splitdoc are not public API, therefore the docstring is interested only for readers of these sources. It can be just a comment. But there is no much sense to just repeat the code by words in a docstring or comment.

There appears to be different ideas at play here surrounding the use docstrings. With one view being docstrings should only document the public API, so users of the API know how to use the package. The other view being docstrings are equally applicable for internal functions, which should have a well defined API for internal use, so internal programmers can tell what the function does without peering into the source. Any external references to help justify one or the other side of this argument appreciated (I have no opinions on this yet myself).

I have also had the out of band comment from Ben saying just because it isn't a public API doesn't make the docstring any less useful. Since a docstring will be available to anyone introspecting the object whether private or public

Docstrings of internal functions are not forbidden, they are just not necessary. Comments would be enough (see PEP 8).

In any case neither comment, nor docstring shouldn't just repeat the code. They should describe what function does, but not how it does this. Otherwise any change of the code will make a docstring or a comment incorrect.

Note also that the function name itself serves the documenting function.

How about:

def short_description_from_docstring(text): # Return the summary line from a docstring. # If there is no summary line, return None. ...

The use of the word "summary line" comes straight from PEP-257.

serhiy-storchaka · 2017-05-09T10:58:17Z

Lib/unittest/test/test_case.py

+    @unittest.skipIf(
+        sys.flags.optimize >= 2,
+        "Docstrings are omitted with -O2 and above")
+    def testShortDescriptionWithSurroundingSpaceOneLineDocstring(self):


I think it is enough to just add spaces or newlines in the two preceding tests.

Sorry, not sure I understand. You appear to be saying that we should use the same docstring as for the testShortDescriptionWithOneLineDocstring test but with extra white space. However it is a different test - doesn't that mean it needs a different doc string to describe how it is different from the previous docstring?

I mean that adding spaces at the start and end of the docstring of testShortDescriptionWithOneLineDocstring and adding a newline and indentation at the start of the docstring of testShortDescriptionWithMultiLineDocstring will cover cases tested by new testes. No new tests will be needed.

Oh, OK, I think I understand now. So to clarify, you feel there is no need to have a separate test for the no-leading/trailing whitespace case?

brettcannon · 2017-05-09T19:15:12Z

FYI Ben Finney has signed the CLA, although this is not a trivial change if you make any changes to the PR yourself, @brianmay then we will need you to sign the CLA, so please do so.

serhiy-storchaka · 2017-05-10T08:56:46Z

Lib/unittest/case.py

+    """
+    Return the test case short description from a docstring.
+
+    Ths docstring is parsed by the standard `pydoc.splitdoc` function


Docstrings of internal functions are not forbidden, they are just not necessary. Comments would be enough (see PEP 8).

In any case neither comment, nor docstring shouldn't just repeat the code. They should describe what function does, but not how it does this. Otherwise any change of the code will make a docstring or a comment incorrect.

Note also that the function name itself serves the documenting function.

serhiy-storchaka · 2017-05-10T08:57:59Z

Lib/unittest/case.py

-        doc = self._testMethodDoc
-        return doc and doc.split("\n")[0].strip() or None
-
+        """ Return a one-line description of the test, if any; otherwise None. """


shortDescription() is a public API. Your change removes useful information from its docstring.

Not sure that the previous description is entirely relevant. Also does saying "The default implementation" add any useful information? How about:

def shortDescription(self): """Returns a one-line description of the test, or None if no description has been provided. This method returns the summary line of the specified test method's docstring, as per PEP-257. """ ...

serhiy-storchaka · 2017-05-10T09:02:36Z

Lib/unittest/test/test_case.py

+    @unittest.skipIf(
+        sys.flags.optimize >= 2,
+        "Docstrings are omitted with -O2 and above")
+    def testShortDescriptionWithSurroundingSpaceOneLineDocstring(self):


I mean that adding spaces at the start and end of the docstring of testShortDescriptionWithOneLineDocstring and adding a newline and indentation at the start of the docstring of testShortDescriptionWithMultiLineDocstring will cover cases tested by new testes. No new tests will be needed.

brianmay · 2017-05-10T23:51:18Z

I have signed the CLA - is it possible to remove "CLA not signed" tag?

brettcannon · 2017-05-11T18:03:50Z

Ah, I see the issue. Your PR has code from https://github.com/benf-wspdigital who has not signed the CLA, so the bot is blocking it as the provenance of the code isn't CLA-clean. Any way of getting that git user to sign the CLA?

brianmay · 2017-05-11T21:07:30Z

I am confused; that looks like Ben Finney's account, and you previously said he had signed the CLA.

encukou · 2017-05-11T22:52:41Z

The bot doesn't recognize him because he has not added his Github username to his profile at bugs.python.org.

brettcannon · 2017-05-12T02:11:26Z

@encukou is right. Ben Finney has signed based on his bugs.python.org account, but the bot nor I can verify that benf-swpdigital on GitHub is Ben Finney.

brianmay · 2017-05-12T04:35:29Z

https://bugs.python.org/msg293522

encukou · 2017-05-12T08:26:39Z

Ben Finney has signed based on his bugs.python.org account, but the bot nor I can verify that benf-swpdigital on GitHub is Ben Finney.

GitHub determines attribution by the e-mail in the commit. This is just text, and easily spoofable – notice how the commits here ended up attributed to Ben without any action on his part, and without GitHub's knowledge of what's on b.p.o.
So, you can tell benf-swpdigital on GitHub registered with the e-mail address in these commits.

Anyway, instead you probably want to verify the patches here are the ones from Ben, i.e. compare them to the ones on b.p.o.

brettcannon · 2017-05-12T16:14:08Z

Just an FYI, this whole CLA issue of people being nice and opening PRs on other people's behalf is being discussed at python/devguide#195 .

terryjreedy · 2017-05-12T19:09:21Z

There would have been less confusion about who has not signed the CLA if the Knight's explanatory message began with "Hello :".

brettcannon · 2017-05-12T19:24:48Z

@terryjreedy python/the-knights-who-say-ni#42

cjerdonek · 2017-05-13T17:41:55Z

Lib/unittest/case.py

@@ -338,6 +339,25 @@ def __exit__(self, exc_type, exc_value, tb):
                .format(logging.getLevelName(self.level), self.logger.name))


+def short_description_from_docstring(text):


This function name should start with an underscore since it's not meant to be part of the public API. Also, I think a shorter name should be used like _make_short_description(docstring) or _get_short_description(docstring). Information about the arguments can be gotten from the argument names or function docstring.

brettcannon · 2017-05-18T22:05:20Z

I talked with our lawyer and he said that as long as Ben and Brian have both signed the CLA then we are fine. So @brianmay can you just tell us your bugs.python.org username and I can manually check you also signed and then this PR will be cleared from a CLA perspective.

brianmay · 2017-07-20T22:21:43Z

@Mariatta said above "I checked that @brianmay has signed the CLA in b.p.o. His b.p.o username is brian".

I don't think there is any need for me to sign the CLA again after it has already been confirmed that I have signed it.

vstinner · 2017-07-20T22:40:52Z

@Mariatta said above "I checked that @brianmay has signed the CLA in b.p.o. His b.p.o username is brian". I don't think there is any need for me to sign the CLA again after it has already been confirmed that I have signed it.

I see that http://bugs.python.org/user14312 has "Contributor Form Received | Yes", bpo login "brian", GitHub login "brianmay".

But I just removed the "CLA not signed" label, and the bot ... added it again, as you didn't sign it :-(

I only see one bpo user with github login="brianmay".

Could it be a mismatch based on the mail address?

@brettcannon: Would you mind to take a look please?

Mariatta · 2017-07-20T23:04:40Z

@Haypo This PR is kinda unique.. What happened is that @brianmay made the PR based on Ben Finney's patch. Because of this the bot tries to check if both Ben and Brian have signed the CLA.
In https://bugs.python.org/msg293522, Ben Finney stated that he does not want to create GitHub account. Since he does not have GitHub account, the bot can't check whether he signed the CLA or not.

I see that both Ben and Brian have signed the CLA. I think from CLA perspective it's clear.
But wouldn't hurt if both you and/or @brettcannon take another look and confirm that both users have signed the CLA.

Otherwise, If the conflict can be resolved, and if you approve this PR, I think this is good to go.

vstinner · 2017-07-20T23:11:53Z

I suggest to export the change as a patch (git diff master.. > patch), create a new PR (git checkout -b pydoc; git apply patch; git commit -a), and credit Ben Finney in the commit message, like:

Co-Authored-By: Ben Finney <xxx@xxx>

cjerdonek · 2017-07-20T23:19:26Z

On the PR itself, note that I have review comments from a couple months ago that haven't been addressed.

brettcannon · 2017-07-20T23:44:32Z

Both Ben and Brian have signed the CLA and it's just Ben's refusal to set up a GitHub account with his bugs.python.org account that's causing the CLA bot to say this PR isn't in the clear. So as long as there are no other commits from anyone but Ben and Brian (which is what is currently true), then from a CLA perspective we're in the clear in this case.

@cjerdonek can you change your review to "request changes" if you think they should be addressed before this PR is considered for acceptance? Plus there are merge conflicts that have to be fixed.

cjerdonek

For backwards compatibility reasons, the PR should preserve the behavior that docstrings of the form 'abc\ndef' return the first line. It should also add a test for this case. While the PR can help ensure that PEP-257 compliant docstrings result in a useful result, it shouldn't require docstrings to be compliant to do so. (Many test-case docstrings out in the wild aren't.)

My suggestion would be to change the implementation of _get_short_description() to match the current implementation, with a call to strip() beforehand. The docstring of _get_short_description() should also be updated accordingly.

brettcannon · 2018-02-02T22:04:57Z

To try and help move older pull requests forward, we are going through and backfilling 'awaiting' labels on pull requests that are lacking the label. Based on the current reviews, the best we can tell in an automated fashion is that a core developer requested changes to be made to this pull request.

If/when the requested changes have been made, please leave a comment that says, I have made the requested changes; please review again. That will trigger a bot to flag this pull request as ready for a follow-up review.

cjerdonek · 2020-10-25T21:48:27Z

I'm closing this as it was separately resolved by PR #18175 for https://bugs.python.org/issue39450

benf-wspdigital added 4 commits May 9, 2017 11:54

Add test cases for single-line test case docstring.

e77fa7a

Add test cases for stripping surrounding space for shortDescription.

538ab19

Add test cases for test case docstring with surrounding newlines.

cec5371

PEP 257 specifies that tools which parse docstrings should strip surrounding blank lines *before* determining what is the first line <URL:https://www.python.org/dev/peps/pep-0257/#handling-docstring-indentation>.

Parse the test case docstring using pydoc.splitdoc.

d1315d4

the-knights-who-say-ni added the CLA not signed label May 9, 2017

brianmay changed the title ~~Wip/issue/issue30181 parse docstring using pydoc~~ bpo-30181 parse docstring using pydoc May 9, 2017

brianmay changed the title ~~bpo-30181 parse docstring using pydoc~~ bpo-30181: parse docstring using pydoc May 9, 2017

serhiy-storchaka reviewed May 9, 2017

View reviewed changes

serhiy-storchaka reviewed May 10, 2017

View reviewed changes

brettcannon removed the CLA not signed label May 11, 2017

the-knights-who-say-ni added the CLA not signed label May 11, 2017

brettcannon removed the CLA not signed label May 11, 2017

the-knights-who-say-ni added the CLA not signed label May 11, 2017

terryjreedy mentioned this pull request May 12, 2017

Explain requirements to convert someone else's patch on b.p.o to a PR python/devguide#195

Closed

cjerdonek reviewed May 13, 2017

View reviewed changes

vstinner removed the CLA not signed label Jul 20, 2017

the-knights-who-say-ni added the CLA not signed label Jul 20, 2017

cjerdonek requested changes Jul 21, 2017

View reviewed changes

Mariatta added needs rebase and removed needs rebase labels Oct 9, 2017

brettcannon added the awaiting changes label Feb 2, 2018

Mariatta removed the CLA not signed label Jun 15, 2018

the-knights-who-say-ni added the CLA not signed label Jun 15, 2018

miss-islington removed the CLA not signed label Aug 28, 2018

the-knights-who-say-ni added the CLA not signed label Aug 28, 2018

brettcannon removed the CLA not signed label Mar 22, 2019

the-knights-who-say-ni added the CLA not signed label Mar 22, 2019

brettcannon closed this Mar 26, 2019

brettcannon reopened this Mar 26, 2019

the-knights-who-say-ni added CLA not signed and removed CLA not signed labels May 16, 2019

brettcannon removed the CLA not signed label Aug 8, 2019

the-knights-who-say-ni added the CLA not signed label Aug 8, 2019

brettcannon added CLA signed and removed CLA not signed labels Aug 8, 2019

the-knights-who-say-ni added the CLA not signed label Aug 8, 2019

cjerdonek closed this Oct 25, 2020

		@@ -338,6 +339,25 @@ def __exit__(self, exc_type, exc_value, tb):
		.format(logging.getLevelName(self.level), self.logger.name))


		def short_description_from_docstring(text):

bpo-30181: parse docstring using pydoc #1505

bpo-30181: parse docstring using pydoc #1505

Conversation

brianmay commented May 9, 2017 • edited by bedevere-bot Loading

the-knights-who-say-ni commented May 9, 2017

brianmay commented May 9, 2017

brianmay commented May 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brianmay May 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brettcannon commented May 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brianmay May 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brianmay commented May 10, 2017

brettcannon commented May 11, 2017

brianmay commented May 11, 2017

encukou commented May 11, 2017

brettcannon commented May 12, 2017

brianmay commented May 12, 2017

encukou commented May 12, 2017

brettcannon commented May 12, 2017

terryjreedy commented May 12, 2017

brettcannon commented May 12, 2017

Choose a reason for hiding this comment

brettcannon commented May 18, 2017

brianmay commented Jul 20, 2017

vstinner commented Jul 20, 2017

Mariatta commented Jul 20, 2017

vstinner commented Jul 20, 2017

cjerdonek commented Jul 20, 2017

brettcannon commented Jul 20, 2017

cjerdonek left a comment

Choose a reason for hiding this comment

brettcannon commented Feb 2, 2018

cjerdonek commented Oct 25, 2020

brianmay commented May 9, 2017 •

edited by bedevere-bot

Loading

brianmay May 10, 2017 •

edited

Loading

brianmay May 11, 2017 •

edited

Loading