ruff icon indicating copy to clipboard operation
ruff copied to clipboard

SIM103 return condition directly incorrect error message

Open IronCore864 opened this issue 1 year ago • 2 comments

I have searched SIM103 in the open issues and didn't find anything related, so creating this one.

It seems the error message/prompt for SIM103 can be incorrect in some cases.

My code base is quite large, but if I copy the single function out for testing, I could not reproduce. It only happens on the large code base:

lint: commands[0]> ruff check --preview
ops/testing.py:3469:9: SIM103 Return the condition `keys is not None and notice.key not in keys` directly
     |
3467 |           if types is not None and notice.type not in types:
3468 |               return False
3469 |           if keys is not None and notice.key not in keys:
     |  _________^
3470 | |             return False
3471 | |         return True
     | |___________________^ SIM103
     |
     = help: Inline condition

Found 1 error.
No fixes available (1 hidden fix can be enabled with the `--unsafe-fixes` option).

I think the prompt/error message is not correct. How could:

if condition:
    return False

return True

be replaced by return condition? The message should be something like "return not condition instead", not "return condition instead". If the user copy/paste the suggestion it'd be wrong.

Only

if condition:
    return True

return False

can be replaced directly by return condition instead.

With latest ruff version 0.3.5.

IronCore864 avatar Apr 09 '24 02:04 IronCore864

In general, you can replace:

if condition:
    return False
return True

With:

return not condition

So in your case, it could be:

return not (keys is not None and notice.key not in keys)

Or, simplified:

return keys is None or notice.key in keys

charliermarsh avatar Apr 09 '24 02:04 charliermarsh

I think the OP is pointing out that the diagnostic message sort of implies you can replace the code with return condition, whereas actually in this case you need return not condition, and the diagnostic doesn't clarify this distinction.

carljm avatar Apr 09 '24 14:04 carljm

Makes sense -- I'll invert it.

charliermarsh avatar Apr 10 '24 02:04 charliermarsh