Skip to content

Fix test decorator for PyLinter options #5195

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 2 commits into from
Oct 22, 2021
Merged

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
βœ“ πŸ› Bug fix

Description

I stumbled upon this while working on #5194.

Without this we don't actually set settings that come from PyLinter correctly and therefore we can't test them with get_global_option.
This should pass tests immediately, but if not I will fix them!

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.

Shouldn't this be done in make_options directly ? It's hard to know you need to do that after calling, we may as well do it all the time automatically ?

@DanielNoord
Copy link
Collaborator Author

The decorator now handles three cases "correctly".

  1. If the option is from the checker that is loaded by the unittest class, it sets the setting using the normal validator and flow
  2. If the option is from PyLinter, it sets the setting using the validator by getting the optdict from PyLinter
  3. If the option is from another checker, it sets the setting using a shortcut. It doesn't create an optdict but is allowed to set the setting

The initial change I did some weeks ago was to allow both 2 and 3, but it turned I should have separated those cases more clearly.

Investing more time in 3 and allowing double-loading of checkers during unittests does not seem worth the effort. If we are going to rework the config handling because of optparse's deprecation anyway, we might as well leave this hacky trick. It's only for our own tests and it is already much better than the decorator we used a couple weeks ago.

@DanielNoord DanielNoord marked this pull request as ready for review October 21, 2021 21:55
@coveralls
Copy link

Pull Request Test Coverage Report for Build 1369823526

  • 4 of 4 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.001%) to 93.25%

Totals Coverage Status
Change from base Build 1360083361: 0.001%
Covered Lines: 13635
Relevant Lines: 14622

πŸ’› - Coveralls

@DanielNoord
Copy link
Collaborator Author

DanielNoord commented Oct 21, 2021

Shouldn't this be done in make_options directly ? It's hard to know you need to do that after calling, we may as well do it all the time automatically ?

make_options is only used twice. Once to merge (I think) command line options with options from PyLinter itself. See L528 in the PyLinter file. For this use we don't need to pass a key as we are expecting it to just return the options dict.

https://github.com/PyCQA/pylint/blob/259f7a381d19bdbfaa84343825e0638323aed55f/pylint/lint/pylinter.py#L527-L528

The other use is on this decorator. Basically we want to call PyLinter.options, but since we can't (since we use UnittestLinter instead of the normal linter) we need to call the constructor of PyLinter.options. Afterwards we immediately pass the key, because we know which optdict we want to "read".

@Pierre-Sassoulas Pierre-Sassoulas added the Maintenance Discussion or action around maintaining pylint or the dev workflow label Oct 22, 2021
@Pierre-Sassoulas Pierre-Sassoulas added this to the 2.12.0 milestone Oct 22, 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.

πŸ‘

@DanielNoord DanielNoord merged commit 772b3dc into main Oct 22, 2021
@DanielNoord DanielNoord deleted the DanielNoord-patch-1 branch October 22, 2021 21:46
@DanielNoord DanielNoord mentioned this pull request Oct 25, 2021
4 tasks
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