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

Fix assignment-from-none false negative case using list.sort() #5738

Merged
merged 4 commits into from
Feb 2, 2022

Conversation

orSolocate
Copy link
Contributor

Type of Changes

Type
βœ“ πŸ› Bug fix
✨ New feature
πŸ”¨ Refactoring
πŸ“œ Docs

Description

Fix assignment-from-none false negative case using list.sort()

Closes #5722

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.

Thank you ! LGTM, I only have some typing and formatting nitpicks

pylint/checkers/typecheck.py Outdated Show resolved Hide resolved
pylint/checkers/typecheck.py Outdated Show resolved Hide resolved
pylint/checkers/typecheck.py Outdated Show resolved Hide resolved
@Pierre-Sassoulas Pierre-Sassoulas added the False Negative πŸ¦‹ No message is emitted but something is wrong with the code label Jan 28, 2022
@Pierre-Sassoulas Pierre-Sassoulas added this to the 2.13.0 milestone Jan 28, 2022
@orSolocate
Copy link
Contributor Author

@Pierre-Sassoulas Thanks for the quick review! Will clear the pipeline errors and apply your suggestions.

@orSolocate orSolocate force-pushed the orb_assign_sort_list branch from d1a786b to 1a8d787 Compare January 29, 2022 12:07
@coveralls
Copy link

coveralls commented Jan 29, 2022

Pull Request Test Coverage Report for Build 1782765241

  • 16 of 16 (100.0%) changed or added relevant lines in 1 file are covered.
  • 30 unchanged lines in 2 files lost coverage.
  • Overall coverage increased (+0.05%) to 93.93%

Files with Coverage Reduction New Missed Lines %
pylint/checkers/classes/class_checker.py 13 94.84%
pylint/checkers/variables.py 17 96.32%
Totals Coverage Status
Change from base Build 1780124211: 0.05%
Covered Lines: 14839
Relevant Lines: 15798

πŸ’› - Coveralls

Copy link
Collaborator

@DanielNoord DanielNoord left a comment

Choose a reason for hiding this comment

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

Thanks @orSolocate! Another great addition. Just some comments about style and order. Fix itself is quite elegant πŸ˜„

pylint/checkers/typecheck.py Outdated Show resolved Hide resolved
pylint/checkers/typecheck.py Outdated Show resolved Hide resolved
pylint/checkers/typecheck.py Outdated Show resolved Hide resolved
pylint/checkers/typecheck.py Outdated Show resolved Hide resolved
pylint/checkers/typecheck.py Outdated Show resolved Hide resolved
pylint/checkers/typecheck.py Outdated Show resolved Hide resolved
pylint/checkers/typecheck.py Outdated Show resolved Hide resolved
@orSolocate orSolocate force-pushed the orb_assign_sort_list branch from 1a8d787 to 7429c46 Compare February 1, 2022 21:18
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.

LGTM, thank you for this, it will definitely help newbies and distracted veterans alike.

pylint/checkers/typecheck.py Outdated Show resolved Hide resolved
self.add_message("assignment-from-no-return", node=node)
else:
for rnode in returns:
for ret_node in return_nodes:
Copy link
Member

Choose a reason for hiding this comment

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

We could even rename it to return_node, I don't suggest because an IDE refactor should do it instantly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You added it yourself already while merging?

Copy link
Member

Choose a reason for hiding this comment

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

It wasn't that important, so I merged :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
False Negative πŸ¦‹ No message is emitted but something is wrong with the code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

assign list.sort() instead of sorted(list)
4 participants