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

Enable usage of custom pylintrc file for message documentation tests #6131

Merged

Conversation

DudeNr33
Copy link
Collaborator

@DudeNr33 DudeNr33 commented Apr 2, 2022

To ease the process of reviewing your PR, do make sure to complete the following boxes.

  • Write a good description on what the PR does.
  • If you used multiple emails or multiple names when contributing, add your mails
    and preferred name in script/.contributors_aliases.json

Type of Changes

Type
🐛 Bug fix
✨ New feature
🔨 Refactoring
📜 Docs

Description

In order to test some messages (e.g. for messages of optional checkers), we need a way to use a custom config.
With this modification it is possible to place a pylintrc file next to good.py and bad.py.
To prove it works I added a documentation example for one of the optional checkers.

Ref: #5953 (comment)

… and demonstrate it with optional checker message as example.
@coveralls
Copy link

coveralls commented Apr 2, 2022

Pull Request Test Coverage Report for Build 2082161191

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 67 unchanged lines in 5 files lost coverage.
  • Overall coverage increased (+0.3%) to 94.54%

Files with Coverage Reduction New Missed Lines %
pylint/checkers/similar.py 3 96.35%
pylint/config/init.py 10 70.31%
pylint/lint/run.py 13 75.52%
pylint/config/option_manager_mixin.py 17 91.24%
pylint/checkers/variables.py 24 96.54%
Totals Coverage Status
Change from base Build 2081862023: 0.3%
Covered Lines: 15514
Relevant Lines: 16410

💛 - Coveralls

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.

Thanks you @DudeNr33 !

@@ -0,0 +1,2 @@
Following an indented ``if`` block directly with an ``elif`` of lower indentation can lead to confusion whether the unindent is deliberate or accidental.
Adding an explicit ``else`` in the indented ``if`` statement, even if it only contains a ``pass`` statement, can help clarify the code.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Adding an explicit ``else`` in the indented ``if`` statement, even if it only contains a ``pass`` statement, can help clarify the code.
Creating a function for the nested conditional, or adding an explicit ``else`` in the indented ``if`` statement, even if it only contains a ``pass`` statement, can help clarify the code.

config_file = next(config.find_default_config_files(), None)
# Check if this message has a custom configuration file (e.g. for enabling optional checkers).
# If not, use the default configuration.
config_file: Optional[Path]
Copy link
Member

Choose a reason for hiding this comment

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

@DanielNoord I think we talked about this already before but what do you think about using FunctionalTestFile instead here ? At some point we will encounter the same issue than for functional tests (i.e. handling min_version, max_version, some options...). We could do test_file.option_file instead of recreating the logic a little differently.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That was actually my initial approach, but the problem is that FunctionalTestFile assumes the config file to have the same name as the .py file.
There is quite a good portion of code duplication also in the LintModuleTest class here.

Besides the different folder structure, the tight coupling between the "original" LintModuleTest and test_functional.py is another problem.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, I advise against trying to use LintModuleTest here. The idea is similar but because of the different folder structures it doesn't really work.

It's similar to your Duck vs. PlasticDuck example @Pierre-Sassoulas 😄

@Pierre-Sassoulas Pierre-Sassoulas merged commit 236313a into pylint-dev:main Apr 2, 2022
@DudeNr33 DudeNr33 deleted the message-doc-test-pylintrc branch April 2, 2022 16:06
@Pierre-Sassoulas Pierre-Sassoulas removed this from the 2.14.0 milestone May 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants