DOC: Improve GL03 message re: blank lines at end of docstrings.

[douglatornell]

re: conversation with @datapythonista in issue #23632

• tests added / passed
• passes git diff upstream/master -u -- "*.py" | flake8 --diff

[pep8speaks]

Hello @douglatornell! Thanks for submitting the PR.

• There are no PEP8 issues in the file scripts/validate_docstrings.py !

[datapythonista]

Did you run the tests python -m pytest scripts? Not sure if that specific error message is checked, but would be worth checking, as the CI will take some time.

Also, if you feel like it, may be you can add a test for the blank line at the end, to make sure we're giving that error. And if we don't have one with the two blank lines, you can add it too.

But happy to merge this PR as it is too, if no test is failing.

[codecov]

Codecov Report

Merging #23649 into master will increase coverage by 0.05%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master   #23649      +/-   ##
==========================================
+ Coverage   92.23%   92.28%   +0.05%     
==========================================
  Files         161      161              
  Lines       51408    51451      +43     
==========================================
+ Hits        47416    47484      +68     
+ Misses       3992     3967      -25

Flag	Coverage Δ
#multiple	90.68% <ø> (+0.05%)	⬆️
#single	42.28% <ø> (-0.02%)	⬇️

Impacted Files	Coverage Δ
pandas/core/arrays/datetimelike.py	95.96% <0%> (-0.19%)	⬇️
pandas/io/formats/format.py	97.76% <0%> (-0.12%)	⬇️
pandas/core/arrays/categorical.py	95.35% <0%> (ø)	⬆️
pandas/core/ops.py	94.27% <0%> (+0.01%)	⬆️
pandas/core/generic.py	96.84% <0%> (+0.01%)	⬆️
pandas/io/parsers.py	95.57% <0%> (+0.01%)	⬆️
pandas/core/indexes/period.py	93.07% <0%> (+0.03%)	⬆️
pandas/core/arrays/datetimes.py	98.51% <0%> (+0.03%)	⬆️
pandas/core/indexes/datetimes.py	96.2% <0%> (+0.05%)	⬆️
pandas/core/arrays/timedeltas.py	96.09% <0%> (+0.45%)	⬆️
... and 3 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1250500...07ed212. Read the comment docs.

[douglatornell]

Thanks for the review!

Yes, I ran the tests locally to make sure that I hadn't broken anything, and they passed with the exception of 4(?) that are marked as expected failures.

I will take a look tomorrow at the possibility of adding test(s) and get update this PR one way or the other.

[datapythonista]

lgtm, I'd merge and add tests for this in a separate PR

[douglatornell]

Sorry @datapythonista, I didn't see your message about adding the tests in a new branch/PR before I pushed them into this one. How do you want me to proceed from here?

[datapythonista]

Don't worry, the changes are also all right here, doesn't make a difference, as this is not yet merged.

I think your tests are saying that does docstrings are correct (which is not the case). But I don't see them failing in the CI. I'm in my phone and can't check well right now, but I think the tests should say that the error messages are generated.

[douglatornell]

I added 2 methods to the BadGenericDocStrings class. The methods have bad docstrings that we expect to produce errors (as do the other methods in that class). The methods are test by test_bad_generic_functions() which asserts that there are errors. I confirmed that that test_bad_generic_functions() fails when the docstrings in the added methods are corrected to not have the extra line breaks.

It's all kind of inverted, but it is in the style of the test module, and resulted in a smaller diff than if I had added a new docstring collection class and new test method (which is what I started to do, but stopped as I gained understanding of the test module).

[datapythonista]

Sorry, my bad. From the phone I couldn't check well, and I thought they were checked with the test_good_docstrings.

Ideally I'd test them with test_bad_examples, so we not only assert that they are wrong, but we also assert that they generate the right error messages.

[douglatornell]

Okay, I'll take a look at putting them in test_bad_examples tomorrow (I hope).

[datapythonista]

@douglatornell do you have time to make the update to the tests?

[douglatornell]

@datapythonista yes. I have a flight later today and am hoping to do them then.

[douglatornell]

@datapythonista bad linebreak tests moved into test_bad_examples() and CI looks happy.

[datapythonista]

Thanks for the update @douglatornell. I'll ask one more change here if you don't mind.

There is something wrong in the tests (not your code, but in master) that is the name test_bad_examples. While the class BadExamples refers to the examples section of the docstrings, the name test_bad_examples refer to testing all the wrong things in any section.

So, if I can ask you to move back the two new examples of line breaks to BadGenericDocStrings, as they don't refer to the examples section. And then rename test_bad_examples to a better name, so it's not confusing, that would be great.

Thanks!

CC @WillAyd

[datapythonista]

Sorry @douglatornell, I think I wasn't clear in my last comment.

Moving the methods with the docstrings back to BadGenericDocStrings is correct.

But the code to call the tests (in the parametrization of test_bad_examples) was already right, and those didn't have to be moved. Instead, what I proposed was to rename the name of the function of the test. As test_bad_examples suggest that only the examples sections is tested there, which is not the case.

Does this make sense?

[douglatornell]

@datapythonista I have reverted the commit that put the new extra linebreak tests into test_bad_examples().

I was going to change the name of test_bad_examples() to test_bad_examples_section(), but looking at its huge pytest.mark.parametrize() decorator, I realize that it also tests other docstring sections; e.g. "See Also". So, maybe it should be test_bad_docstring_sections(). But then I note that the decorator also contains BadGenericDocStrings and I look above it and see the test_bad_generic_functions() test...

I don't know the history of the test_validate_docstrings module, nor if you have big goal in mind for these docstring clean-up issues, but I am wondering if test_validate_docstrings needs a wholesale refactoring?

[datapythonista]

The validate_docstrings.py script almost contains everything we have in mind now. Just few validations missing. So, no big goal in mind.

I don't think the tests need a major refactoring. May be merging the two tests you mention can make sense (I didn't check in detail), but other than that I think we should be all right.

I'd rename test_bad_examples to test_bad_docstrings in this PR, and if you think more changes would make things better, I'd open another PR for that.

What do you think?

[datapythonista]

Thanks for the update @douglatornell, looks perfect now.

[jreback]

thanks @douglatornell