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

Remove TODOs in unused-variable tests #6162

Merged
merged 2 commits into from
Apr 3, 2022

Conversation

jacobtylerwalls
Copy link
Member

Type of Changes

Type
βœ“ πŸ”¨ Refactoring

Description

Per discussion on #4391, which we closed, I think we can stick to the one-warning-per-variable-per-scope contract, so these TODOs are out of date.

If we want to look into adding warnings, we can do it more broadly in #5838.

Further discussion suggested making no changes (one warning per name per scope).
@jacobtylerwalls jacobtylerwalls added the Maintenance Discussion or action around maintaining pylint or the dev workflow label Apr 3, 2022
@jacobtylerwalls jacobtylerwalls added this to the 2.14.0 milestone Apr 3, 2022
# pylint: disable=redefined-outer-name, wrong-import-position,misplaced-future
# pylint: disable=wrong-import-position,misplaced-future
Copy link
Member Author

Choose a reason for hiding this comment

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

I thought it was worth surfacing the redefined-outer-name in these tests so you can more easily see that some warning is raised for the fishiness.

@jacobtylerwalls jacobtylerwalls changed the title Remove TODOs in unused_variable tests Remove TODOs in unused-variable tests Apr 3, 2022
@coveralls
Copy link

coveralls commented Apr 3, 2022

Pull Request Test Coverage Report for Build 2085453651

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 11 unchanged lines in 1 file lost coverage.
  • Overall coverage increased (+0.0003%) to 94.411%

Files with Coverage Reduction New Missed Lines %
pylint/checkers/variables.py 11 96.54%
Totals Coverage Status
Change from base Build 2085347247: 0.0003%
Covered Lines: 15540
Relevant Lines: 16460

πŸ’› - 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.

Nice cleanup !

@jacobtylerwalls
Copy link
Member Author

I'm looking into the behavior change -- see pylint run. Whether I have to restore the special case or not I'll add a functional test.

@jacobtylerwalls
Copy link
Member Author

Hmm. Whatever it is belongs in another PR. Haven't narrowed it down yet. I'll restore the code I thought was useless, and then this can keep moving.

@Pierre-Sassoulas Pierre-Sassoulas merged commit 0880d95 into pylint-dev:main Apr 3, 2022
@jacobtylerwalls jacobtylerwalls deleted the remove-todo branch April 3, 2022 14:24
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.

None yet

3 participants