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

expression-not-assigned should not be emitted for ternary operator #2380

Closed
sushobhit27 opened this issue Aug 4, 2018 · 3 comments
Closed

Comments

@sushobhit27
Copy link
Contributor

Steps to reproduce

  1. cat sample.py
def goo(var):
    pass

def bar(var):
    pass

def foo():
    goo(True) if True else bar(False)
  1. $ pylint sample.py --disable=blacklisted-name,unused-argument,using-constant-test

Current behavior

************* Module sample
sample.py:8:4: W0106: Expression "goo(True) if True else bar(False)" is assigned to nothing (expression-not-assigned)


Your code has been rated at 8.33/10 (previous run: 8.33/10, +0.00)

Expected behavior

There should be no warning when some functionality has to be executed using ternary operator.
Actuall use case, where this message is being raised: https://github.com/sushobhit27/bimap/blob/8b28e6501b96872c109e0a51e4f7e6081bec0c8f/bimap/bimap.py#L57

pylint --version output

$ pylint --version
pylint 2.0.0
astroid 2.0.1
Python 3.6.4 (default, Jan 7 2018, 15:53:53)
[GCC 6.4.0]

@PCManticore
Copy link
Contributor

@sushobhit27 I think it's reasonable that this check gets emitted here. This would be better written as an if statement, which is a bit clearer and easier to follow.

if True:
   run_this()
else:
   run_that()

@sushobhit27
Copy link
Contributor Author

@PCManticore here is another use case where this check should not be emitted IMO
https://github.com/sushobhit27/multiindex/blob/1679b8d86040a8f05b4f117abe21244cf9a1d48e/multiindex/multiindex.py#L71

can_be_removed = [index.can_be_removed(deleter, obj) for index in self.indexes.values()]
        if all(can_be_removed):
            [index.remove(getattr(obj, index.index_name)) for index in self.indexes.values()]

I know it can be re-written as

can_be_removed = [index.can_be_removed(deleter, obj) for index in self.indexes.values()]
        if all(can_be_removed):
            for index in self.indexes.values():
                index.remove(getattr(obj, index.index_name))

but that increases the indentation and this particular style I have seen in some third party packages also, e.g

https://github.com/networkx/networkx/blob/3d97582466ee42df47bb791fe7948f9009316c0b/networkx/algorithms/hybrid.py#L173

@PCManticore
Copy link
Contributor

But in your case you are creating a transient list that just gets thrown away after you're done with the iteration. It's more efficient to just use the for loop here, and it goes without saying that it helps with the readability of the code as well.

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

No branches or pull requests

2 participants