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.

Sign up for GitHub

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

Jump to bottom

Make unused-import check all ancestors for typing guards #5316

Merged
merged 1 commit into from Nov 25, 2021
Merged

Make unused-import check all ancestors for typing guards #5316

merged 1 commit into from Nov 25, 2021

Conversation

DanielNoord
Copy link
Collaborator

  • 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
✓ 🐛 Bug fix

Description

See pylint-dev/astroid#1235 (comment)

@DanielNoord DanielNoord added the False Positive 🦟 A message is emitted but nothing is wrong with the code label Nov 15, 2021
@DanielNoord DanielNoord added this to the 2.12.2 milestone Nov 15, 2021
Pierre-Sassoulas
Pierre-Sassoulas approved these changes Nov 15, 2021
View reviewed changes
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.

I'm glad it's for 2.12.2 as 2.12.0 just reach 200 issues, and I like round number 😄

pylint/checkers/variables.py Show resolved Hide resolved
@coveralls
Copy link

coveralls commented Nov 15, 2021 •
edited

Pull Request Test Coverage Report for Build 1499398248

  • 7 of 7 (100.0%) changed or added relevant lines in 1 file are covered.
  • 134 unchanged lines in 16 files lost coverage.
  • Overall coverage increased (+0.08%) to 93.477%
Files with Coverage Reduction New Missed Lines %
pylint/checkers/stdlib.py 1 93.45%
pylint/config/option.py 1 83.02%
pylint/extensions/mccabe.py 1 99.05%
pylint/checkers/utils.py 2 95.4%
pylint/extensions/overlapping_exceptions.py 2 84.09%
pylint/message/message.py 2 86.67%
pylint/testutils/reporter_for_tests.py 3 93.48%
pylint/checkers/exceptions.py 5 97.27%
pylint/checkers/base_checker.py 7 85.57%
pylint/testutils/lint_module_test.py 9 74.7%
Totals Coverage Status
Change from base Build 1464202583: 0.08%
Covered Lines: 13957
Relevant Lines: 14931
💛 - Coveralls

@Pierre-Sassoulas Pierre-Sassoulas added the Blocked 🚧 Blocked by a particular issue label Nov 15, 2021
@DanielNoord
Copy link
Collaborator Author

Closing and opening to run the primer on this!

@DanielNoord DanielNoord reopened this Nov 24, 2021
@Pierre-Sassoulas Pierre-Sassoulas removed the Blocked 🚧 Blocked by a particular issue label Nov 25, 2021
@Pierre-Sassoulas Pierre-Sassoulas merged commit fe69c1d into pylint-dev:main Nov 25, 2021
@DanielNoord
Copy link
Collaborator Author

No changelog entry? Hadn't added it yet

@DanielNoord DanielNoord deleted the import branch November 25, 2021 17:06
@Pierre-Sassoulas
Copy link
Member

No changelog entry? Hadn't added it yet

I supposed it was voluntary as in "this is a change small enough to not be in the changelog", my bad.

@DanielNoord
Copy link
Collaborator Author

DanielNoord commented Nov 25, 2021 •
edited

Ah no worries. I think this should go in the changelog though. Could you add an entry? I'm away from computer atm.

@Pierre-Sassoulas
Copy link
Member

Sure, no problem !

@Pierre-Sassoulas Pierre-Sassoulas mentioned this pull request Nov 25, 2021
Merged
@Pierre-Sassoulas
Copy link
Member

I figured I'd do that in #5397 instead of opening a MR specifically for this.

cdce8p
cdce8p reviewed Nov 27, 2021
View reviewed changes
pylint/checkers/variables.py
Comment on lines +364 to +365
if isinstance(ancestor, nodes.If):
if ancestor.test.as_string() in TYPING_TYPE_CHECKS_GUARDS:
Copy link
Member

Choose a reason for hiding this comment

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

I'm again late, as I'm going through my notifications.
Can't those two be combined? Technically even a return any(...) would be possible, but I think that might be more difficult to read.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, should have catched that myself. Going to make a PR with this change!

consider-combining-if seems like a handy checker for our review process 😄

Copy link
Member

Choose a reason for hiding this comment

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

consider-combining-if seems like a handy checker for our review process 😄

That is also somewhere on my ideas list 😅

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I found the any to be okay for readability. See #5410

Copy link
Member

Choose a reason for hiding this comment

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

consider-combining-if seems like a handy checker for our review process

Why didn't I think of that sooner, It's like half the content of my reviews 😄 Also it should trigger on thing like any(bool(e == 1) for e in elements if int(e) == e)). Let me open an issue.

Copy link
Member

Choose a reason for hiding this comment

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

#5414

@DanielNoord DanielNoord mentioned this pull request Nov 27, 2021
Merged
4 tasks
@Pierre-Sassoulas Pierre-Sassoulas mentioned this pull request Nov 28, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Pierre-Sassoulas Pierre-Sassoulas Pierre-Sassoulas approved these changes

@cdce8p cdce8p cdce8p left review comments

Assignees
No one assigned
Labels
False Positive 🦟 A message is emitted but nothing is wrong with the code
Projects
None yet
Milestone
2.12.2
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@DanielNoord @coveralls @Pierre-Sassoulas @cdce8p