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

Improve singleton-comparison suggestion message #3820

Merged

Conversation

ethan-leba
Copy link
Contributor

@ethan-leba ethan-leba commented Sep 7, 2020

Description

Singleton-comparison will now suggest is for boolean constants, as well as a message for truthiness/falsiness in case the user intended this, whereas the message would only suggest checking truthiness/falsiness before. The message will now also display the actual code in question, instead of an expr stand-in.

Type of Changes

Type
🐛 Bug fix
✨ New feature
🔨 Refactoring
📜 Docs

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.09%) to 90.64% when pulling 26de6e4 on ethan-leba:simplifiable-condition-backup into 7ff83e2 on PyCQA:master.

@coveralls
Copy link

coveralls commented Sep 7, 2020

Coverage Status

Coverage decreased (-0.004%) to 90.718% when pulling 0d3ec6b on ethan-leba:simplifiable-condition-backup into e602fda on PyCQA:master.

@ethan-leba ethan-leba marked this pull request as ready for review September 7, 2020 16:27
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 like this change. I don't like that the "utils.py" file is getting bigger because for me "utils" are where we we put things we can't name properly. But I don't have a smart name to suggest, so... Can it be used elsewhere ?

pylint/checkers/base.py Outdated Show resolved Hide resolved
tests/functional/s/singleton_comparison.txt Outdated Show resolved Hide resolved
@ethan-leba
Copy link
Contributor Author

Thanks for the quick review! In regards to the utils file, I'm not too sure what the best course of action there is either. I don't think it makes sense to keep those functions in the len-checker file if we're importing it in a bunch of different places though. Maybe the utils file could be split into a package and categorized in another PR? Possibly as a stopgap we could add a utils module to the new refactoring package for now

@Pierre-Sassoulas
Copy link
Member

Pierre-Sassoulas commented Sep 7, 2020

Maybe the utils file could be split into a package and categorized in another PR?

Yeah, probably. We've started doing that before. Maybe those utility function should be in Astroid instead. Anyway I have no good alternative ideas and this is not the main focus of the PR anyway. Moving them to a more generic file like you did is good enough for now :)

@ethan-leba
Copy link
Contributor Author

Not in the scope of this current PR, but should we be checking for Enum members as well for singleton-comparison?

@Pierre-Sassoulas Pierre-Sassoulas added this to the 2.6.1 release milestone Sep 14, 2020
@ethan-leba ethan-leba force-pushed the simplifiable-condition-backup branch 2 times, most recently from 09f5bb7 to 4aaad0d Compare September 20, 2020 21:27
@ethan-leba
Copy link
Contributor Author

This PR should be ready for re-review now @Pierre-Sassoulas 🙂

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 some nitpicky comments but the result is awesome, thank you !

self.add_message(
"singleton-comparison", node=root_node, args=(True, suggestion)
def _check_singleton_comparison(
self, lhs_value, rhs_value, root_node, checking_for_absence=False
Copy link
Member

Choose a reason for hiding this comment

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

Could we had typing for modified functions ?

Suggested change
self, lhs_value, rhs_value, root_node, checking_for_absence=False
self, lhs_value, rhs_value, root_node, checking_for_absence:bool =False

"singleton-comparison", node=root_node, args=(False, suggestion)

if _is_singleton_const(lhs_value):
singleton, other_value = lhs_value.value, rhs_value
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we can rename lhs to left_value and rhs to right value ? Is it an acronym for left hand side ?

singleton-comparison:13::Comparison to True should be just 'not expr'
singleton-comparison:14::Comparison to False should be 'expr'
singleton-comparison:15::Comparison to None should be 'expr is not None'
singleton-comparison:4::Comparison 'x == None' should be 'x is None'
Copy link
Member

Choose a reason for hiding this comment

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

👌

@Pierre-Sassoulas Pierre-Sassoulas self-assigned this Sep 22, 2020
@Pierre-Sassoulas Pierre-Sassoulas added the Enhancement ✨ Improvement to a component label Sep 22, 2020
@ethan-leba
Copy link
Contributor Author

Added in the changes, let me know if there's anything else that needs work before merging! Also, for my previous PR I think I might've placed documentation incorrectly, since I added to the 'What's New' and 'ChangeLog' for v2.6 after it had already been merged. Should I move this documentation to the v2.7 section?

@Pierre-Sassoulas Pierre-Sassoulas merged commit 339f16f into pylint-dev:master Sep 22, 2020
@Pierre-Sassoulas
Copy link
Member

Thank you for this merge request, this message is most useful for python's beginners; so a very clear message is helping a lot.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement ✨ Improvement to a component
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants