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

re: conversation with @datapythonista in issue #23632

- [x] tests added / passed
- [x] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`


Labels:
name: Docs
name: CI
description: Continuous Integration

Comments:
Hello @douglatornell! Thanks for submitting the PR.

 - There are no PEP8 issues in the file [`scripts/validate_docstrings.py`](https://github.com/douglatornell/pandas/blob/a73ab815c385e38430c927b67b63d23b65ce37f2/scripts/validate_docstrings.py) !


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](https://codecov.io/gh/pandas-dev/pandas/pull/23649?src=pr&el=h1) Report
> Merging [#23649](https://codecov.io/gh/pandas-dev/pandas/pull/23649?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/1250500cfe18f60cfb8a4867c82d14467bbee7ad?src=pr&el=desc) will **increase** coverage by `0.05%`.
> The diff coverage is `n/a`.

[![Impacted file tree graph](https://codecov.io/gh/pandas-dev/pandas/pull/23649/graphs/tree.svg?width=650&token=eZ4WkYLtcO&height=150&src=pr)](https://codecov.io/gh/pandas-dev/pandas/pull/23649?src=pr&el=tree)

```diff
@@            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%)` | :arrow_up: |
| #single | `42.28% <ø> (-0.02%)` | :arrow_down: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/23649?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/core/arrays/datetimelike.py](https://codecov.io/gh/pandas-dev/pandas/pull/23649/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvYXJyYXlzL2RhdGV0aW1lbGlrZS5weQ==) | `95.96% <0%> (-0.19%)` | :arrow_down: |
| [pandas/io/formats/format.py](https://codecov.io/gh/pandas-dev/pandas/pull/23649/diff?src=pr&el=tree#diff-cGFuZGFzL2lvL2Zvcm1hdHMvZm9ybWF0LnB5) | `97.76% <0%> (-0.12%)` | :arrow_down: |
| [pandas/core/arrays/categorical.py](https://codecov.io/gh/pandas-dev/pandas/pull/23649/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvYXJyYXlzL2NhdGVnb3JpY2FsLnB5) | `95.35% <0%> (ø)` | :arrow_up: |
| [pandas/core/ops.py](https://codecov.io/gh/pandas-dev/pandas/pull/23649/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvb3BzLnB5) | `94.27% <0%> (+0.01%)` | :arrow_up: |
| [pandas/core/generic.py](https://codecov.io/gh/pandas-dev/pandas/pull/23649/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZ2VuZXJpYy5weQ==) | `96.84% <0%> (+0.01%)` | :arrow_up: |
| [pandas/io/parsers.py](https://codecov.io/gh/pandas-dev/pandas/pull/23649/diff?src=pr&el=tree#diff-cGFuZGFzL2lvL3BhcnNlcnMucHk=) | `95.57% <0%> (+0.01%)` | :arrow_up: |
| [pandas/core/indexes/period.py](https://codecov.io/gh/pandas-dev/pandas/pull/23649/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvaW5kZXhlcy9wZXJpb2QucHk=) | `93.07% <0%> (+0.03%)` | :arrow_up: |
| [pandas/core/arrays/datetimes.py](https://codecov.io/gh/pandas-dev/pandas/pull/23649/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvYXJyYXlzL2RhdGV0aW1lcy5weQ==) | `98.51% <0%> (+0.03%)` | :arrow_up: |
| [pandas/core/indexes/datetimes.py](https://codecov.io/gh/pandas-dev/pandas/pull/23649/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvaW5kZXhlcy9kYXRldGltZXMucHk=) | `96.2% <0%> (+0.05%)` | :arrow_up: |
| [pandas/core/arrays/timedeltas.py](https://codecov.io/gh/pandas-dev/pandas/pull/23649/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvYXJyYXlzL3RpbWVkZWx0YXMucHk=) | `96.09% <0%> (+0.45%)` | :arrow_up: |
| ... and [3 more](https://codecov.io/gh/pandas-dev/pandas/pull/23649/diff?src=pr&el=tree-more) | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/23649?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/23649?src=pr&el=footer). Last update [1250500...07ed212](https://codecov.io/gh/pandas-dev/pandas/pull/23649?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

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.
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?
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.
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).
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.
Okay, I'll take a look at putting them in `test_bad_examples` tomorrow (I hope).
@douglatornell do you have time to make the update to the tests?
@datapythonista yes. I have a flight later today and am hoping to do them then.
@datapythonista bad linebreak tests moved into `test_bad_examples()` and CI looks happy.
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 
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?
@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?
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?
thanks @douglatornell