Skip to content
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

Add ignored-parents option to design checker #4758

Merged
merged 10 commits into from Jul 28, 2021

Conversation

9999years
Copy link
Contributor

@9999years 9999years commented Jul 27, 2021

Type of Changes

Type
✨ New feature

Description

Allow users to specify classes to ignore while counting parent classes.

When one of the ignored classes is detected in the ancestor tree, it and its parents are ignored. Suppose we have the following inheritance diagram:

       F
      /
  D  E
   \/
    B  C
     \/
      A      # class A(B, C): ...

If we add E to ignored-parents, then the classes counted for the too-many-ancestors check will be B, D, and C.

If we have another class G(F), then F will be counted -- ancestors of ignored parents are pruned from the tree for counting, but we don't go the other way and exclude all parents of ignored parents.

Partially closes #3057 by offering a configuration mechanism to ignore non-user classes from counting.

Rationale

typing.Protocol is ignored from counting, but I'm using typing_extensions, so typing_extensions.Protocol is increasing the count and triggering the too-many-ancestors message.

This allows users to specify classes to ignore while counting parent
classes.

Partially closes pylint-dev#3057
@9999years 9999years marked this pull request as ready for review July 27, 2021 16:51
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.

Thank you for the merge request and the added option, this looks nice. I left some comments. Could you also add a new functional test in tests/functional/.../too_many_ancestors.py with a configuration file, please ?

ChangeLog Outdated Show resolved Hide resolved
pylint/checkers/design_analysis.py Show resolved Hide resolved
pylint/checkers/design_analysis.py Outdated Show resolved Hide resolved
tests/checkers/unittest_design.py Outdated Show resolved Hide resolved
tests/checkers/unittest_design.py Show resolved Hide resolved
@coveralls
Copy link

coveralls commented Jul 27, 2021

Coverage Status

Coverage increased (+0.007%) to 92.161% when pulling 29d6efd on 9999years:feature/ignore-parents into e04de25 on PyCQA:main.

@9999years
Copy link
Contributor Author

Added a functional test -- should I keep the tests/checkers/unittest_design.py file I added or is just the functional test OK?

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.

Thank you for implementing the functional tests ! It's true that the unittest could be a duplicate now. I don't know for sure if it's covering more line than simply the functional test.

@9999years
Copy link
Contributor Author

I'll leave it -- it won't hurt and it'll make it easier for the next person who wants to unit test the design checker.

@Pierre-Sassoulas Pierre-Sassoulas added False Positive 🦟 A message is emitted but nothing is wrong with the code Checkers Related to a checker Enhancement ✨ Improvement to a component and removed False Positive 🦟 A message is emitted but nothing is wrong with the code labels Jul 27, 2021
@Pierre-Sassoulas Pierre-Sassoulas added this to the 2.10.0 milestone Jul 27, 2021
@Pierre-Sassoulas
Copy link
Member

There will be a slight delay before merging as we want to release 2.9.6 first and this is going in 2.10 🙂

@Pierre-Sassoulas Pierre-Sassoulas merged commit 24d03e9 into pylint-dev:main Jul 28, 2021
@Pierre-Sassoulas
Copy link
Member

Thank you again for your work on this, this will be available in 2.10.0 !

@9999years 9999years deleted the feature/ignore-parents branch July 28, 2021 21:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Checkers Related to a checker Enhancement ✨ Improvement to a component
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Count ancestors only in user code
3 participants