Skip to content

Move some of the Numpy tests out of TestParamDocChecker #5492

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 21 commits into from
Dec 8, 2021

Conversation

DanielNoord
Copy link
Collaborator

  • Add yourself to CONTRIBUTORS if you are a new contributor.
  • Write a good description on what the PR does.

Type of Changes

Type
βœ“ πŸ”¨ Refactoring

Description

This does not cover all numpy tests as there are some issues with those that check raise documentation (it seems as if tests are incorrectly passing currently).
Just to make the other PR easier to review, let's merge this first!

@DanielNoord DanielNoord added the Maintenance Discussion or action around maintaining pylint or the dev workflow label Dec 8, 2021
@coveralls
Copy link

Pull Request Test Coverage Report for Build 1555034207

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 93.696%

Totals Coverage Status
Change from base Build 1550052772: 0.0%
Covered Lines: 14164
Relevant Lines: 15117

πŸ’› - Coveralls

@Pierre-Sassoulas Pierre-Sassoulas added this to the 2.13.0 milestone Dec 8, 2021
Copy link
Member

@Pierre-Sassoulas Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

πŸŽ‰ 100 lines less to maintain, again ! Great work !

@@ -0,0 +1,6 @@
"""Tests for missing-param-doc and missing-type-doc for Numpy style docstrings
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm curious, do you create this commit at the very end by rebasing and taking the value you had after moving everything ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Taken from missing_param_doc_required_Google as they tend to have the same tests and thus similar ignores. I don't think there are any unnecessary ignores in here (and it doesn't really hurt if there are as long as they are not the messages we want to test)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ho alright, thank you for explaining :)

@@ -25,7 +25,6 @@
in particular the parameter documentation checker `DocstringChecker`
"""

# pylint: disable=too-many-public-methods
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

πŸ‘Œ

@DanielNoord DanielNoord merged commit a51a548 into pylint-dev:main Dec 8, 2021
@DanielNoord DanielNoord deleted the move-unittests-18 branch December 8, 2021 18:42
@DanielNoord
Copy link
Collaborator Author

@Pierre-Sassoulas I took another look at the remaining numpy tests. Those are actually incorrectly passing indeed.
Do you want me to do a fix PR and move PR or can I do the moving of the tests and fix in the same PR?

Basically, the numpy checker doesn't recognise property's correctly and recognises none of the raise documentation.

@Pierre-Sassoulas
Copy link
Member

Nice job finding the problem in tests ! Moving then fixing would permit to have a better history by just squashing both, but if it takes too much time I'm okay with mixing too.

@DanielNoord
Copy link
Collaborator Author

Moving>fixing is (nearly) impossible as I can't let the tests pass without fixing or accepting false positives.

I'll have a look somewhere this week what the best effort/reward balance would be.

@Pierre-Sassoulas
Copy link
Member

Ok, I see the problem now, let's do both at the same time then, there's no point in fixing then moving it's more work for only theoretical benefit.

@DanielNoord
Copy link
Collaborator Author

@Pierre-Sassoulas The issue is more troublesome than I thought:
https://github.com/PyCQA/pylint/blob/a51a5486ebff7543ae4fb6943fac2558947fa974/pylint/extensions/_check_docs_utils.py#L529-L536

This is a pretty naive approach at validating the docstrings. What happens for the remaining tests is that the property_returns_lines matches the Google style as it is similar to Numpy. Therefore this method returns True, even though the raises sections does not match the Google style.

https://github.com/PyCQA/pylint/blob/a51a5486ebff7543ae4fb6943fac2558947fa974/pylint/extensions/_check_docs_utils.py#L175-L184
Because of the order of this tuple we match with Google before Numpy and thus never actually see that the docstring we're testing is not Google, but `Numpy.

To fix this we need to change how we validate docstrings. Something that takes into account when we match multiple types and then fixes the conflict between them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Maintenance Discussion or action around maintaining pylint or the dev workflow
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants