New checker: comparison to empty string constant #1183

Merged
merged 1 commit into from Dec 29, 2016

Conversation

Projects
None yet
3 participants
@atodorov
Contributor

atodorov commented Nov 30, 2016

Detects things like

if X == '':
    pass

Note that in some cases this is what the author needs but my experience tells me most of the times empty string value doesn't have a special meaning.

Note2: this probably belongs to the ComparisonChecker in base.py. This checker however only supports binary comparisons so I will have to fix his limitation first. Let me know how would you like to proceed about that.

@rowillia

This comment has been minimized.

Show comment
Hide comment
@rowillia

rowillia Dec 1, 2016

Contributor

@atodorov Thanks for the PR! I've got to think this one over a bit. I've definitely had code where I compare to empty string as shorthand for x is not None and isinstance(x, str), it's especially handy in parsing code.

If we go forward with this one I'd like to see us turn it off by default.

Contributor

rowillia commented Dec 1, 2016

@atodorov Thanks for the PR! I've got to think this one over a bit. I've definitely had code where I compare to empty string as shorthand for x is not None and isinstance(x, str), it's especially handy in parsing code.

If we go forward with this one I'd like to see us turn it off by default.

@atodorov

This comment has been minimized.

Show comment
Hide comment
@atodorov

atodorov Dec 1, 2016

Contributor

@rowillia - others to think about are if X != 0, where X is an int, X is None, X == None, X is not None, X != None

Contributor

atodorov commented Dec 1, 2016

@rowillia - others to think about are if X != 0, where X is an int, X is None, X == None, X is not None, X != None

@rowillia

This comment has been minimized.

Show comment
Hide comment
@rowillia

rowillia Dec 2, 2016

Contributor

@atodorov The len change is fairly uncontroversial. The only False-y value len can return is 0.

In these cases where we don't know the type of X, it could be perfectly valid to compare to 0 if, say, X could be None or 0. While I think it's a terrible pattern, I've frequently seen code that does things like say None means no limit, 0 means the limit is 0.

Contributor

rowillia commented Dec 2, 2016

@atodorov The len change is fairly uncontroversial. The only False-y value len can return is 0.

In these cases where we don't know the type of X, it could be perfectly valid to compare to 0 if, say, X could be None or 0. While I think it's a terrible pattern, I've frequently seen code that does things like say None means no limit, 0 means the limit is 0.

@PCManticore

This comment has been minimized.

Show comment
Hide comment
@PCManticore

PCManticore Dec 3, 2016

Member

I agree with @rowillia that this might make sense only in some situations. I think this can become an extension, since we don't have full support for disabled checks (only checker classes).

Member

PCManticore commented Dec 3, 2016

I agree with @rowillia that this might make sense only in some situations. I think this can become an extension, since we don't have full support for disabled checks (only checker classes).

@atodorov

This comment has been minimized.

Show comment
Hide comment
@atodorov

atodorov Dec 10, 2016

Contributor

@PCManticore @rowillia refactored into an extension so that people can enable it if they so desire. (I certainly have a need for these checkers).

Can you comment on the code/docs, etc so I can proceed with the other examples I've listed in the comment above?

Contributor

atodorov commented Dec 10, 2016

@PCManticore @rowillia refactored into an extension so that people can enable it if they so desire. (I certainly have a need for these checkers).

Can you comment on the code/docs, etc so I can proceed with the other examples I've listed in the comment above?

doc/whatsnew/2.0.rst
+
+ You can activate this checker by adding the line::
+
+ load-plugins=pylint.extensions.docparams

This comment has been minimized.

@PCManticore

PCManticore Dec 14, 2016

Member

Probably you mean emptystring here?

@PCManticore

PCManticore Dec 14, 2016

Member

Probably you mean emptystring here?

pylint/extensions/emptystring.py
+
+
+def _is_constant_empty_str(node):
+ return isinstance(node, astroid.Const) and isinstance(node.value, str) and node.value == ''

This comment has been minimized.

@PCManticore

PCManticore Dec 14, 2016

Member

You can drop the middle check, since the last one makes it redundant.

@PCManticore

PCManticore Dec 14, 2016

Member

You can drop the middle check, since the last one makes it redundant.

pylint/extensions/emptystring.py
+ __implements__ = (interfaces.IAstroidChecker,)
+
+ # configuration section name
+ name = 'cmp-empty-str'

This comment has been minimized.

@PCManticore

PCManticore Dec 14, 2016

Member

compare-to-empty-string? Longer, but more intuitive.

@PCManticore

PCManticore Dec 14, 2016

Member

compare-to-empty-string? Longer, but more intuitive.

pylint/extensions/emptystring.py
+ error_detected = False
+
+ # x ?? ""
+ if _is_constant_empty_str(op_1) and op_2 in ['!=', '==', 'is not', 'is']:

This comment has been minimized.

@PCManticore

PCManticore Dec 14, 2016

Member

Let's reuse this list, move it into a separate variable.

@PCManticore

PCManticore Dec 14, 2016

Member

Let's reuse this list, move it into a separate variable.

@PCManticore

This comment has been minimized.

Show comment
Hide comment
@PCManticore

PCManticore Dec 14, 2016

Member

@atodorov In general, it looks good, apart of a couple of small nits.

Member

PCManticore commented Dec 14, 2016

@atodorov In general, it looks good, apart of a couple of small nits.

@atodorov

This comment has been minimized.

Show comment
Hide comment
@atodorov

atodorov Dec 20, 2016

Contributor

@PCManticore should be good to go now.

Contributor

atodorov commented Dec 20, 2016

@PCManticore should be good to go now.

@PCManticore PCManticore merged commit 97a6a4f into PyCQA:master Dec 29, 2016

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.01%) to 90.686%
Details
@PCManticore

This comment has been minimized.

Show comment
Hide comment
Member

PCManticore commented Dec 29, 2016

Thank you @atodorov

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment