Test in scripts/validate_docstrings.py that the short summary is always one line long #22617
Conversation
|
Hello @Moisan! Thanks for submitting the PR.
|
scripts/validate_docstrings.py
Outdated
| @@ -163,7 +163,7 @@ def double_blank_lines(self): | |||
|
|
|||
| @property | |||
| def summary(self): | |||
| if not self.doc['Extended Summary'] and len(self.doc['Summary']) > 1: | |||
| if len(self.doc['Summary']) > 1: | |||
There was a problem hiding this comment.
Hmm what's the point of this condition at all? Haven't stepped through in detail but seems like it might just be an erroneous copy / paste from extended_summary?
There was a problem hiding this comment.
My feeling is also that it was an erroneous copy from extended_summary. The current check returns an error only when the summary is empty hence my change here.
I could probably simply return self.doc['Summary'] for this method and add another @property to obtain the number of lines of the summary.
There was a problem hiding this comment.
Can you just return the summary here and update the check to detect multiple lines? Don't want to get too far off topic here but raising an error on an empty string seems rather arbitrary
…ines in validate_docstrings script
Codecov Report
@@ Coverage Diff @@
## master #22617 +/- ##
=======================================
Coverage 92.04% 92.04%
=======================================
Files 169 169
Lines 50782 50782
=======================================
Hits 46744 46744
Misses 4038 4038
Continue to review full report at Codecov.
|
| return ' '.join(self.doc['Summary']) | ||
|
|
||
| @property | ||
| def num_summary_lines(self): |
There was a problem hiding this comment.
Overall approach is much cleaner here, though I'm maybe -0.5 on the property as I don't see this really being used outside of the one check you have, in which case you can probably just do len(self.doc['Summary']) directly
I'll defer to the @datapythonista on this one though
|
@datapythonista pls merge if ok |
|
thanks @Moisan |
git diff upstream/master -u -- "*.py" | flake8 --diffThe previous test to check if the summary property should be empty was based on the non-existence of the extended summary which doesn't seem to make sense. The test case I added failed before changing the script.