DOC: Add validation error when a docstring is missing #23669
Conversation
|
Hello @avolkov! Thanks for updating the PR.
Comment last updated on November 13, 2018 at 16:28 Hours UTC |
| @@ -668,7 +668,7 @@ class IndexOpsMixin(object): | |||
| __array_priority__ = 1000 | |||
|
|
|||
| def transpose(self, *args, **kwargs): | |||
| """ return the transpose, which is by definition self """ | |||
| """ Return the transpose, which is by definition self """ | |||
There was a problem hiding this comment.
This is orthogonal to the change right? Would be better to keep these in a separate PR focused on fixing up these docstrings, especially given there are other issues with the docstrings (ex: missing period)
There was a problem hiding this comment.
Yes, sorry about that, this got merged from the other branch I've been working on.
I added a revert commit, but I'm going to look for a way of removing the commit entirely
There was a problem hiding this comment.
I've tried to do a rebase removing the commits from local tree that I then force-pushed to remote. That didn't seem to remove offending commits from PR.
@WillAyd do you mind if I create a new pull request once the rest of the issues have been rectified?
| @@ -668,6 +668,12 @@ def missing_whitespace_after_comma(self): | |||
| pass | |||
|
|
|||
|
|
|||
| class NoDocstrings(object): | |||
There was a problem hiding this comment.
Does this need to be a dedicated class or can we just add this method to the existing class with bad docstrings?
There was a problem hiding this comment.
It's not a bad docstring It's missing docstring, out of these, each of the classes describe particular bad docstring behaviour:
- BadGenericDocstrings -- badly phrased docstrings
- BadSummarries -- badly summarized docstrings
- BadParameters -- badly documented parameters
- BadReturns -- bad return docs
- BadSeeAlso -- badly formatted see also section
- BadExamples -- bad examples section
I guess this method might go to BadGenericDocstrings, but none of the candidates for it seem ideal.
| @@ -720,6 +726,13 @@ def test_bad_class(self): | |||
| assert isinstance(errors, list) | |||
| assert errors | |||
|
|
|||
| @capture_stderr | |||
| def test_bad_class(self): | |||
There was a problem hiding this comment.
Hmm most of our focus in on docstrings for methods. Not sure we explicitly require the class to have a doctoring though. @datapythonista
There was a problem hiding this comment.
I forgot to rename function i've been using as an example. will fix
| def test_bad_class(self): | ||
| errors = validate_one(self._import_path( | ||
| klass='NoDocstrings'))['errors'] | ||
| assert isinstance(errors, list) |
There was a problem hiding this comment.
If we need this (though not sure we do) would be better to have a stronger assertion
There was a problem hiding this comment.
on the second thought this test is not strong enough, adding a test case in test_bad_generic function should be sufficient.
There was a problem hiding this comment.
an argument for leaving it in (with the fixed name) -- this class will give more informative error message in case NoDocstring class is removed from test source.
|
|
||
| if len(doc.raw_doc) == 0: | ||
| errs.append(error('GL08')) | ||
| return { |
There was a problem hiding this comment.
Do we need to explicitly return here? Would be more inline with rest of the code if we allow it to pass over the remaining conditions (which would assumedly be all False anyway) and return at the end of the function
There was a problem hiding this comment.
The rest of the condition will not be False, if we don't do early return the next two conditions would evaluate to true
doc.start_blank_lines
doc.end_blank_lines
it's possible to put the code underneath into all-encompassing else statement, but the early return just seems cleaner.
Closes pandas-dev#22215 SparseArray.take not accepting scalars is already in 0.24.0.txt
|
New PR is fine. I'll close this one out |
git diff upstream/master -u -- "*.py" | flake8 --diff