Skip to content

Move tests from TestParamDocChecker to functional tests #5509

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 12 commits into from
Dec 13, 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

Last move for TestParamDocChecker πŸ˜„

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

coveralls commented Dec 13, 2021

Pull Request Test Coverage Report for Build 1572457590

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 11 unchanged lines in 2 files lost coverage.
  • Overall coverage decreased (-0.07%) to 93.641%

Files with Coverage Reduction New Missed Lines %
pylint/testutils/checker_test_case.py 1 96.77%
pylint/testutils/decorator.py 10 63.64%
Totals Coverage Status
Change from base Build 1571889854: -0.07%
Covered Lines: 14181
Relevant Lines: 15144

πŸ’› - Coveralls

@DanielNoord
Copy link
Collaborator Author

@Pierre-Sassoulas Do you want me to remove these unused testutils? Or do we want to keep them for future uses?

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.

πŸ‘

@Pierre-Sassoulas
Copy link
Member

Do you want me to remove these unused testutils?

Let's keep them so it's not a breaking change for plugin developer.

@Pierre-Sassoulas Pierre-Sassoulas added this to the 2.13.0 milestone Dec 13, 2021
@DanielNoord
Copy link
Collaborator Author

Let's keep them so it's not a breaking change for plugin developer.

Shall I add a DeprecationWarning? set_config_directly was added in 2.12 (perhaps 2.11) by me as a hacky solution to an issue with multiple checkers using the same config names. If you're using it you should probably redesign your checker πŸ˜„

@Pierre-Sassoulas
Copy link
Member

Sorry I missed that we just added them in 2.12.2 I think we could remove, I doubt anyone jumped on it.

@DanielNoord
Copy link
Collaborator Author

Sorry I missed that we just added them in 2.12.2 I think we could remove, I doubt anyone jumped on it.

It was 2.12, see #5195

Still okay with removing that decorator?

@Pierre-Sassoulas
Copy link
Member

Do you want to revert 5195 ? I hope in the future there won't be the same argument in multiple checker in the configuration so removing it would not be a problem anymore. Meanwhile using functional test all the time has a lot of advantages.

@DanielNoord
Copy link
Collaborator Author

I don't think reverting makes sense as #5195 actually built upon other changes to these decorators.

For now I would propose to deprecate set_config_directly in 2.13 and remove it in 2.14:
If plugin developers are actively developing plugins that they jumped on this after 2.12 I think they should also notice this deprecation. Normally we should obviously remove it in 3.0 but since this is actually allowing badly designed checkers to be tested in a hacky way I don't think we should wait for it.
If you agree I'll merge this and then do a follow-up with the deprecation.

@Pierre-Sassoulas
Copy link
Member

The minor version deprecation makes a lot of sense, let's do that πŸ‘

@DanielNoord DanielNoord merged commit df99320 into pylint-dev:main Dec 13, 2021
@DanielNoord DanielNoord deleted the move-unittests-20 branch December 13, 2021 11:58
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