sourcery-ai / sourcery

Instant AI code reviews
https://sourcery.ai
MIT License
1.51k stars 65 forks source link

`invert-any-all` does not refactor correctly with chained comparison #286

Closed randallpittman closed 1 year ago

randallpittman commented 1 year ago

Checklist

Description

On the following code:

if not all(0 <= v <= 255 for v in v): 
    raise ValueError(...)

Sourcery recommends the following change:

if any((0 <= v <= 255 for v in v)): 
   raise ValueError(...)

citing the invert-any-all rule. It seems that the invert-any-all rule does not know how to handle chained comparisons.

This would be an appropriate application of the invert-any-all rule (though not an improvement in this case, IMO):

if any(v < 0 or 255 < v for v in v):
    raise ValueError(...)

Debug Information

IDE Version: Visual Studio Code 1.71.2

Sourcery Version: v0.12.9

Operating system and Version: Linux Mint 20.3

randallpittman commented 1 year ago

Related, the extra parentheses are not needed--any and all accept the bare comprehension just fine.

ruancomelli commented 1 year ago

Hello, @randallpittman, thanks for reporting this issue!

You are correct in all of your three main points:

  1. replacing not all(0 <= v <= 255 for v in v) with any((0 <= v <= 255 for v in v)) is just wrong
  2. even if Sourcery had suggested the correct change, I agree that any(v < 0 or 255 < v for v in v) is not any clearer
  3. and yes, Sourcery should not have introduced that extraneous pair of parentheses

I was able to reproduce it and will work on a fix.

ruancomelli commented 1 year ago

Hi again, @randallpittman!

A fix for this issue was just merged and will be available in the next release.

Thank you for reporting it!

randallpittman commented 1 year ago

@ruancomelli No problem--thank you for your attention to the issue!

Hellebore commented 1 year ago

Now live!