Skip to content

Conversation

DudeNr33
Copy link
Collaborator

@DudeNr33 DudeNr33 commented Apr 1, 2022

  • 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.
  • 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

This should fix the false positive reported in #6089 (yes I named my branch wrong 😉).
But as I am not very familar with this part of the code base, I would like to ask @jacobtylerwalls for a review if moving this check can have other side effects than just a small performance loss.
Test suite is passing locally.

Closes #6089

@DudeNr33 DudeNr33 requested a review from jacobtylerwalls April 1, 2022 13:34
@Pierre-Sassoulas Pierre-Sassoulas added the False Positive 🦟 A message is emitted but nothing is wrong with the code label Apr 1, 2022
Copy link
Member

@jacobtylerwalls jacobtylerwalls left a comment

Choose a reason for hiding this comment

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

Great analysis. I think this condition and the instance variables it manages should just be deleted. Does it make sense to do it in a follow up PR or here?

If the check is moved even lower to leapfrog over the last CONTINUE action, then it's not accomplishing much. (Even _is_only_type_annotation() is good about short-circuiting.)

@Pierre-Sassoulas Pierre-Sassoulas added this to the 2.13.5 milestone Apr 2, 2022
@Pierre-Sassoulas Pierre-Sassoulas added the Needs backport Needs to be cherry-picked on the current patch version by a pylint's maintainer label Apr 2, 2022
@coveralls
Copy link

coveralls commented Apr 2, 2022

Pull Request Test Coverage Report for Build 2085456668

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.001%) to 94.41%

Totals Coverage Status
Change from base Build 2085390512: -0.001%
Covered Lines: 15537
Relevant Lines: 16457

💛 - Coveralls

@jacobtylerwalls
Copy link
Member

@DudeNr33
Copy link
Collaborator Author

DudeNr33 commented Apr 2, 2022

self._is_used_before_assignment_enabled has been removed with the latest commit.

@DudeNr33
Copy link
Collaborator Author

DudeNr33 commented Apr 3, 2022

@Pierre-Sassoulas how do you handle backports?
Merge the current PR into main and then merge it manually also into the maintenance branch for 2.13?
Should I update the ChangeLog and whatsnew entry to include the change in the sections for 2.13.5 in this PR already?

@Pierre-Sassoulas
Copy link
Member

how do you handle backports?

The full doc

TLDR we squash on the main branch so we only have to cherry-pick one commit on the maintenance branch later. (What is not said is that I cherry-pick everything in chronological order just before the release, so you can just label "Need backport", We could do it just after merging too)

Should I update the ChangeLog and whatsnew entry to include the change in the sections for 2.13.5 in this PR already?

Yes, in the current patch version 2.13.5 for the changelog and the whatsnew for 2.13.

@DudeNr33
Copy link
Collaborator Author

DudeNr33 commented Apr 3, 2022

Thanks for the info! I have updated the ChangeLog and whatsnew/2.13.rst.
But I fear that something went wrong when I tried to merge the current main branch, as it now displays an awful lot of changed files...

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 think it should be a rebase instead of a merge, it's probably fixable with git rebase origin/main.

@Pierre-Sassoulas
Copy link
Member

You need to disregard what git is telling you when you're pushing and actually rewrite the history by using git push --force

@DudeNr33 DudeNr33 force-pushed the fix-6089-unused-import-false-negative branch from 002cbca to 26c47d1 Compare April 3, 2022 13:38
@DudeNr33
Copy link
Collaborator Author

DudeNr33 commented Apr 3, 2022

Thanks a ton, that did the trick. I'm still struggling with git sometimes... 😀

@DudeNr33
Copy link
Collaborator Author

DudeNr33 commented Apr 3, 2022

Thanks @jacobtylerwalls, removed the entries for 2.14.

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 Pierre-Sassoulas merged commit e444a22 into pylint-dev:main Apr 3, 2022
@DudeNr33 DudeNr33 deleted the fix-6089-unused-import-false-negative branch April 3, 2022 15:42
@Pierre-Sassoulas Pierre-Sassoulas added Backported and removed Needs backport Needs to be cherry-picked on the current patch version by a pylint's maintainer labels Apr 4, 2022
Pierre-Sassoulas pushed a commit that referenced this pull request Apr 4, 2022
…efore-assignment`` and ``undefined-variable`` (#6096)

Co-authored-by: Jacob Walls <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Backported False Positive 🦟 A message is emitted but nothing is wrong with the code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

False positive unused-import (W0611) when assigning class properties with the same name
4 participants