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

Make global-variable-not-assigned check local scope #4990

Merged
merged 6 commits into from Sep 11, 2021

Conversation

DanielNoord
Copy link
Collaborator

@DanielNoord DanielNoord commented Sep 10, 2021

  • 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
✨ New feature

Description

This checker now checks whether the names after the global keyword are reassigned in the local scope.
I created a small utils function that checks whether the name appears in any of the AssignName nodes in the scope.

The updated test only throws an error for 2 of the 4 variables. Not quite sure what that is about and we might want to make another issue/PR to fix that. However, since the change did not break any of the tests the file is designed for I guessed it was okay for now. This PR does fix the original issue.

This closes #1375
This closes #330

This checker now checks whether the names after the global keyword
are reassigned in the local scope.
This closes pylint-dev#1375
@coveralls
Copy link

coveralls commented Sep 10, 2021

Pull Request Test Coverage Report for Build 1224244856

  • 5 of 5 (100.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.002%) to 93.08%

Totals Coverage Status
Change from base Build 1223688596: 0.002%
Covered Lines: 13249
Relevant Lines: 14234

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

I have a small nitpick but this is great :)

pylint/checkers/utils.py Outdated Show resolved Hide resolved
@Pierre-Sassoulas Pierre-Sassoulas added Bug 🪲 False Negative 🦋 No message is emitted but something is wrong with the code labels Sep 10, 2021
@Pierre-Sassoulas Pierre-Sassoulas added this to the 2.11.0 milestone Sep 10, 2021
DanielNoord and others added 4 commits September 10, 2021 22:01
Co-authored-by: Pierre Sassoulas <pierre.sassoulas@gmail.com>
This checker now also checks function defintions in the module and local scope
This closes pylint-dev#330
@DanielNoord
Copy link
Collaborator Author

Sorry for adding changes after your review, but I stumbled upon #330 and it felt weird not to fix it in the same PR.
Basically the latest commit adds additional checks for function redefinitions (both on previous constants and functions) similar to how the first iteration checked for variable assignments.

Because #330 also indicated that function definition in the module scope were not picked up on by the check for global-variable-undefined I also added it so we can close that issue.
Thinking about it that might warrant an additional changelog entry.. Let me know if I should add it!

@Pierre-Sassoulas Pierre-Sassoulas merged commit dcd4887 into pylint-dev:main Sep 11, 2021
@Pierre-Sassoulas
Copy link
Member

Don't worry, thank you for fixing another problem :)

@DanielNoord DanielNoord deleted the global-variable branch September 13, 2021 11:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug 🪲 False Negative 🦋 No message is emitted but something is wrong with the code
Projects
None yet
3 participants