Add configuration option `exclude-too-few-public-methods` #5191

miketheman · 2021-10-20T13:28:46Z

Add yourself to CONTRIBUTORS if you are a new contributor.
Add a ChangeLog entry describing what your PR does.
If it's a new feature, or an important bug fix, add a What's New entry in
doc/whatsnew/<current release.rst>.
Write a good description on what the PR does.

Type of Changes

	Type
✓	✨ New feature

Description

Add feature to exclude classes from too-few-public-methods

Instead of adding specific packages to an in-code exclusion list,
add a configuration value that allows the end user to set values
for packages/modules and classes to exclude from evaluation.

Closes #3370

miketheman

@Pierre-Sassoulas et al - here's what I've come up with so far. Looking forward to your review!

pylint/checkers/design_analysis.py

tests/checkers/unittest_design.py

DanielNoord

I like where this is going! Would be good to see an issue with (relatively) many upvotes get fixed!

I'm happy to assist with any of the changes I suggested!

pylint/checkers/design_analysis.py

tests/checkers/unittest_design.py

ChangeLog

pylint/checkers/design_analysis.py

coveralls · 2021-10-20T17:14:43Z

Pull Request Test Coverage Report for Build 1383302578

6 of 6 (100.0%) changed or added relevant lines in 2 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.002%) to 93.338%

Totals
Change from base Build 1383025181:	0.002%
Covered Lines:	13688
Relevant Lines:	14665

💛 - Coveralls

DanielNoord

We will really need to use get_global_option here to make this option and its implementation future-proof.

A good example of how to do this can be found here:
https://github.com/PyCQA/pylint/blob/80205dc813e17fc416e56bace982c971a10553fb/pylint/checkers/imports.py#L477
get_global_option is used to create a private attribute for which we do know the type. We do this in the open method because then it is called as soon as the checker class is "opened".

We get type inference because we have "overloaded" get_global_option here:
https://github.com/PyCQA/pylint/blob/80205dc813e17fc416e56bace982c971a10553fb/pylint/utils/utils.py#L232-L236

You will notice currently there is no GLOBAL_OPTION_PATTERN_LIST at the top of the file.
Furthermore, in T_GlobalOptionReturnTypes List[Pattern[str]] is not included. https://github.com/PyCQA/pylint/blob/80205dc813e17fc416e56bace982c971a10553fb/pylint/utils/utils.py#L67-L69

So to make this work and prepare it for a type-proof future we will need to do three things:

Add get_global_option() to the open method of the class to create a private attribute and use that attribute in the checker
Add a GLOBAL_OPTION_PATTERN_LIST variable to the utils/utils.py file
Add the return type of GLOBAL_OPTION_PATTERN_LIST options (which is List[Pattern[str]] to T_GlobalOptionReturnTypes
Add GLOBAL_OPTION_PATTERN_LIST to GLOBAL_OPTION_NAMES
Create a new overload based on GLOBAL_OPTION_PATTERN_LIST and its return type

I know this might be a little more than you initially signed up for, so please let me know if I can help with anything or if you would like me to show how this should be done! As I said, I'm happy to help and think this is a valuable addition to pylint!

pylintrc

pylint/checkers/design_analysis.py

DanielNoord · 2021-10-21T21:50:01Z

@miketheman Just to let you know, I actually had to create a list of regex patterns option type myself 😄

It's located in this PR: DanielNoord#20 However, I need #5195 to be merged before I can create a PR for this change.
But to spare yourself some time you might want to wait for my PR to be merged. It should be much easier to add this option to GLOBAL_OPTION_PATTERN_LIST afterwards as I have already done step 2, 3, 4 and 5!

miketheman · 2021-10-23T00:27:16Z

... you might want to wait for my PR to be merged ...

@DanielNoord Thanks for the guidance! I saw the dependent PR get merged, so am in no rush. I think the code works just fine, and no mypy errors are currently emitted, so I'm not clear how to test this if/when I make that change?

pylint/checkers/design_analysis.py

DanielNoord · 2021-10-23T08:49:09Z

@DanielNoord Thanks for the guidance! I saw the dependent PR get merged, so am in no rush. I think the code works just fine, and no mypy errors are currently emitted, so I'm not clear how to test this if/when I make that change?

I'm going to go over some of the review comments I got on some of my own existing PRs and then submit the PR which I talked about. I will split the commits so you can follow along and see what you would need to add once mine has been merged. You could also duplicate those commits here and then we would just resolve merge conflicts once both have been approved!

DanielNoord · 2021-10-23T10:17:46Z

I've cleaned up the PR and the commit history. You can look at it here:
#5201

To make your new option work you will need to add exclude-too-few-public-methods to the Literal list of GLOBAL_OPTION_PATTERN_LIST. This will tell mypy that when exclude-too-few-public-methods is used in get_global_option it should expect a List[Pattern[str]] return. See the relevant commit for my own PR here:
8a8e7a5

Next comes the slightly more difficult task of adding unittests for the new option. You can look at what I did here:
4b52567
You will need to do this in the TestDesignChecker class you already added your previous tests to:
https://github.com/PyCQA/pylint/blob/19b92aa4a717680361f9994110775afa6e76974d/tests/checkers/unittest_design.py#L16
Basically, by calling the set_config decorator you can "emulate" the process of setting a config in pylintrc (or any other config option). So you should create a simple test where the function is decorated with a string like you would normally use in pylintrc and see if get_global_option returns the expected list of compiled patterns. The tests I added in that commit both test the default value (which should be a list even if it is not provided) and the possibility of matching patterns using the any pattern.

You can wait for my PR to be merged or copy the work from 8a8e7a5 and you should be able to add the test yourself. That way you don't need to wait for me and we can just solve possible merge conflicts when the time comes.

Hopefully this explains it well enough, but please let me know if you have any further questions. I know this is all quite a bit of extra work for a simple new option, without any clear previous examples of how to do this. We need to improve our testing of setting configs with tests so this serves as a good learning case for us as well!

DanielNoord · 2021-10-24T17:38:06Z

@miketheman My PR has been merged, so if you merge main into your branch you should be able to built upon it!

miketheman · 2021-10-24T17:51:48Z

I'll revisit soon!

miketheman · 2021-10-24T21:29:47Z

@DanielNoord I was confused about the test failure - I don't see that locally, and looks like I had misnamed a test output file. I've renamed the file in the most recent commit.

DanielNoord

This looks good! Perfect use of the get_global_option util :)

pylint/checkers/design_analysis.py

tests/checkers/unittest_design.py

DanielNoord

Sorry for nitpicking so much on the option description. I believe this is the most true to what it actually does.

The failing tests seem to be a case of adding some disables to the functional test file!

pylint/checkers/design_analysis.py

pylintrc

miketheman · 2021-10-24T21:53:53Z

The failing tests seem to be a case of adding some disables to the functional test file!

I'm not sure I understand this - as the test seems to only fail on Windows and pypy3 - is it possible those envs don't have the yaml/toml packages included?

DanielNoord · 2021-10-24T22:26:17Z

I guess so. Can we change the tests to not require an import? Just by inheriting from another class within the same file and then adding that classes name to the regex?

miketheman · 2021-10-24T22:30:21Z

Sure, I can do that. But I was really trying to embody the capability of the original intent - excluding an imported module's inherited class - is there one from the Python stdlib you think would work better?

DanielNoord · 2021-10-24T22:32:37Z

I can't think of one immediately. Perhaps @Pierre-Sassoulas has some good suggestions here!

Pierre-Sassoulas

Great job 👍

tests/functional/t/too/too_few_public_methods_excluded.py

Before changing anything about the code, make sure we have a unit test that covers the basic behavior. Signed-off-by: Mike Fiedler <[email protected]>

Instead of adding specific packages to an in-code exclusion list, add a configuration value that allows the end user to set values for packages/modules and classes to exclude from evaluation. TODO: needs documentation and examples Signed-off-by: Mike Fiedler <[email protected]>

Signed-off-by: Mike Fiedler <[email protected]>

Also update the help string. Signed-off-by: Mike Fiedler <[email protected]>

Signed-off-by: Mike Fiedler <[email protected]>

Co-authored-by: Daniël van Noord <[email protected]>

for more information, see https://pre-commit.ci

Signed-off-by: Mike Fiedler <[email protected]>

Using a stdlib of json ought to be future-proof enough to traverse platforms. Signed-off-by: Mike Fiedler <[email protected]>

Co-authored-by: Daniël van Noord <[email protected]>

for more information, see https://pre-commit.ci

DanielNoord

Good choice!

Thanks for your work @miketheman! This is a really nice contribution and good and concise PR :)

tests/functional/t/too/too_few_public_methods_excluded.rc

Pierre-Sassoulas

Thanks, very nice evolution !

miketheman commented Oct 20, 2021

View reviewed changes

pylint/checkers/design_analysis.py Outdated Show resolved Hide resolved

pylint/checkers/design_analysis.py Show resolved Hide resolved

tests/checkers/unittest_design.py Outdated Show resolved Hide resolved

DanielNoord requested changes Oct 20, 2021

View reviewed changes

DanielNoord added Enhancement ✨ Improvement to a component Configuration Related to configuration labels Oct 20, 2021

miketheman requested a review from DanielNoord October 20, 2021 23:34

miketheman marked this pull request as ready for review October 20, 2021 23:35

DanielNoord requested changes Oct 21, 2021

View reviewed changes

pylintrc Show resolved Hide resolved

pylint/checkers/design_analysis.py Outdated Show resolved Hide resolved

pylint/checkers/design_analysis.py Outdated Show resolved Hide resolved

DanielNoord reviewed Oct 23, 2021

View reviewed changes

pylint/checkers/design_analysis.py Outdated Show resolved Hide resolved

miketheman force-pushed the miketheman/exclude-class-from-too-few-public branch from 19b92aa to f47e156 Compare October 24, 2021 21:17

miketheman requested review from DanielNoord and Pierre-Sassoulas October 24, 2021 21:17

DanielNoord requested changes Oct 24, 2021

View reviewed changes

pylint/checkers/design_analysis.py Outdated Show resolved Hide resolved

pylint/checkers/design_analysis.py Outdated Show resolved Hide resolved

pylint/checkers/design_analysis.py Outdated Show resolved Hide resolved

tests/checkers/unittest_design.py Outdated Show resolved Hide resolved

miketheman requested a review from DanielNoord October 24, 2021 21:40

DanielNoord requested changes Oct 24, 2021

View reviewed changes

pylint/checkers/design_analysis.py Outdated Show resolved Hide resolved

pylintrc Outdated Show resolved Hide resolved

Pierre-Sassoulas approved these changes Oct 25, 2021

View reviewed changes

tests/functional/t/too/too_few_public_methods_excluded.py Outdated Show resolved Hide resolved

miketheman added 3 commits October 25, 2021 19:07

Add tests for existing too-few-public-methods

90c53d5

Before changing anything about the code, make sure we have a unit test that covers the basic behavior. Signed-off-by: Mike Fiedler <[email protected]>

Add myself as a new contributor

a363b36

Signed-off-by: Mike Fiedler <[email protected]>

miketheman and others added 14 commits October 25, 2021 19:08

Add entry to changelog and whatsnew

411768d

Signed-off-by: Mike Fiedler <[email protected]>

refactor: replace csv type with regexp_csv type

89879ab

Also update the help string. Signed-off-by: Mike Fiedler <[email protected]>

test: add functional test

b86625d

Signed-off-by: Mike Fiedler <[email protected]>

feat: add configuration empty key

2b24558

Signed-off-by: Mike Fiedler <[email protected]>

Refactor to use get_global_option() during open()

b6b9a6e

Signed-off-by: Mike Fiedler <[email protected]>

Update default return value

afd9ccc

Signed-off-by: Mike Fiedler <[email protected]>

Update config to include new exclusion parameter

f06af0d

Signed-off-by: Mike Fiedler <[email protected]>

Remove unit tests covered by functional tests

268fe37

Signed-off-by: Mike Fiedler <[email protected]>

Rename test case file

99c58fb

Signed-off-by: Mike Fiedler <[email protected]>

Apply suggestions from code review

3e1ea42

Co-authored-by: Daniël van Noord <[email protected]>

[pre-commit.ci] auto fixes from pre-commit.com hooks

7af6e07

for more information, see https://pre-commit.ci

Fix list comprehension

64a6f39

Signed-off-by: Mike Fiedler <[email protected]>

test: improve test to support pypy/windows

984c042

Using a stdlib of json ought to be future-proof enough to traverse platforms. Signed-off-by: Mike Fiedler <[email protected]>

Apply suggestions from code review

bbfbffa

Co-authored-by: Daniël van Noord <[email protected]>

miketheman force-pushed the miketheman/exclude-class-from-too-few-public branch from 7f5af68 to bbfbffa Compare October 25, 2021 23:08

miketheman requested review from DanielNoord and Pierre-Sassoulas October 25, 2021 23:08

[pre-commit.ci] auto fixes from pre-commit.com hooks

558609a

for more information, see https://pre-commit.ci

DanielNoord approved these changes Oct 25, 2021

View reviewed changes

tests/functional/t/too/too_few_public_methods_excluded.rc Show resolved Hide resolved

Pierre-Sassoulas approved these changes Oct 26, 2021

View reviewed changes

DanielNoord merged commit 448485a into pylint-dev:main Oct 26, 2021

DanielNoord added this to the 2.12.0 milestone Oct 26, 2021

DanielNoord added the Hacktoberfest label Oct 26, 2021

miketheman deleted the miketheman/exclude-class-from-too-few-public branch October 26, 2021 12:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add configuration option `exclude-too-few-public-methods` #5191

Add configuration option `exclude-too-few-public-methods` #5191

miketheman commented Oct 20, 2021 •

edited

Loading

miketheman left a comment

DanielNoord left a comment

coveralls commented Oct 20, 2021 •

edited

Loading

DanielNoord left a comment

DanielNoord commented Oct 21, 2021

miketheman commented Oct 23, 2021 •

edited

Loading

DanielNoord commented Oct 23, 2021

DanielNoord commented Oct 23, 2021

DanielNoord commented Oct 24, 2021

miketheman commented Oct 24, 2021

miketheman commented Oct 24, 2021

DanielNoord left a comment

DanielNoord left a comment

miketheman commented Oct 24, 2021

DanielNoord commented Oct 24, 2021

miketheman commented Oct 24, 2021 •

edited

Loading

DanielNoord commented Oct 24, 2021

Pierre-Sassoulas left a comment

DanielNoord left a comment

Pierre-Sassoulas left a comment

Add configuration option exclude-too-few-public-methods #5191

Add configuration option exclude-too-few-public-methods #5191

Conversation

miketheman commented Oct 20, 2021 • edited Loading

Type of Changes

Description

miketheman left a comment

Choose a reason for hiding this comment

DanielNoord left a comment

Choose a reason for hiding this comment

coveralls commented Oct 20, 2021 • edited Loading

Pull Request Test Coverage Report for Build 1383302578

💛 - Coveralls

DanielNoord left a comment

Choose a reason for hiding this comment

DanielNoord commented Oct 21, 2021

miketheman commented Oct 23, 2021 • edited Loading

DanielNoord commented Oct 23, 2021

DanielNoord commented Oct 23, 2021

DanielNoord commented Oct 24, 2021

miketheman commented Oct 24, 2021

miketheman commented Oct 24, 2021

DanielNoord left a comment

Choose a reason for hiding this comment

DanielNoord left a comment

Choose a reason for hiding this comment

miketheman commented Oct 24, 2021

DanielNoord commented Oct 24, 2021

miketheman commented Oct 24, 2021 • edited Loading

DanielNoord commented Oct 24, 2021

Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

DanielNoord left a comment

Choose a reason for hiding this comment

Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

Add configuration option `exclude-too-few-public-methods` #5191

Add configuration option `exclude-too-few-public-methods` #5191

miketheman commented Oct 20, 2021 •

edited

Loading

coveralls commented Oct 20, 2021 •

edited

Loading

miketheman commented Oct 23, 2021 •

edited

Loading

miketheman commented Oct 24, 2021 •

edited

Loading