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

ruff doesn't find the same E721 violations as new flake8 does #12290

Closed
timj opened this issue Jul 11, 2024 · 8 comments · Fixed by #12300
Closed

ruff doesn't find the same E721 violations as new flake8 does #12290

timj opened this issue Jul 11, 2024 · 8 comments · Fixed by #12300
Assignees
Labels
bug Something isn't working

Comments

@timj
Copy link

timj commented Jul 11, 2024

I searched for E721 but couldn't find a previous ticket.

ruff 0.5.1

For the file https://github.com/lsst/geom/blob/main/tests/test_coordinates.py flake8 7.1.0 reports this error:

tests/test_coordinates.py:299:16: E721 do not compare types, for exact checks use `is` / `is not`, for instance checks use `isinstance()`

The relevant line is:

if type(result) != expected:

ruff 0.5.1 does not report any E721 violation. There is no ruff configuration in this directory when I clone.

I have other examples of inconsistency. In https://github.com/lsst/fgcm flake8 reports:

/fgcm/fgcmConfig.py:30:20: E721 do not compare types, for exact checks use `is` / `is not`, for instance checks use `isinstance()`
./fgcm/fgcmConfig.py:33:20: E721 do not compare types, for exact checks use `is` / `is not`, for instance checks use `isinstance()`
./fgcm/fgcmConfig.py:59:16: E721 do not compare types, for exact checks use `is` / `is not`, for instance checks use `isinstance()`

ruff reports:

fgcm/fgcmConfig.py:33:20: E721 Use `is` and `is not` for type comparisons, or `isinstance()` for isinstance checks
   |
31 |                     raise TypeError("Default is the wrong datatype.")
32 |             if self._value is not None:
33 |                 if type(self._value) != datatype:
   |                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ E721
34 |                     raise TypeError("Value is the wrong datatype.")
   |

fgcm/fgcmConfig.py:59:16: E721 Use `is` and `is not` for type comparisons, or `isinstance()` for isinstance checks
   |
58 |         if self._datatype is not None:
59 |             if type(self._value) != self._datatype:
   |                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ E721
60 |                 raise ValueError("Datatype mismatch for %s (got %s, expected %s)" %
61 |                                  (name, str(type(self._value)), str(self._datatype)))
   |

fgcm/sharedNumpyMemManager.py:100:15: E721 Use `is` and `is not` for type comparisons, or `isinstance()` for isinstance checks
    |
 98 |         elif (dtype == np.int16):
 99 |             ctype = ctypes.c_int16
100 |         elif (dtype == bool):
    |               ^^^^^^^^^^^^^ E721
101 |             ctype = ctypes.c_bool
102 |         else:
    |

where two of the warnings agree but flake8 is reporting one from line 30 of fgcmConfig.py which ruff does not report, and ruff reports one from sharedNumpyMemManager.py that flake8 does not report.

@timj
Copy link
Author

timj commented Jul 11, 2024

I see #9570 special cases numpy, which explains why line 98 above does not trigger the warning.

@zanieb
Copy link
Member

zanieb commented Jul 11, 2024

Hm I'm not sure why we're not raising a violation there. It looks like a false negative as far as I can tell?

Note some more context on false positives for this rule at #4560

@timj
Copy link
Author

timj commented Jul 11, 2024

A different example from https://github.com/RobertLuptonTheGood/eups.

The code:

return isinstance(self, EupsCmd) and type(self) != EupsCmd

triggers E721 in flake8 but not in ruff.

python/eups/cmd.py:253:46: E721 do not compare types, for exact checks use `is` / `is not`, for instance checks use `isinstance()`

ruff does find 3 places that flake8 doesn't though.

@timj
Copy link
Author

timj commented Jul 11, 2024

I'm also seeing cases where type(x) != type(y) is not triggering a warning in ruff but will in flake8.

@charliermarsh
Copy link
Member

I'm also seeing cases where type(x) != type(y) is not triggering a warning in ruff but will in flake8.

Can you give a concrete example of this?

@charliermarsh charliermarsh self-assigned this Jul 12, 2024
@charliermarsh charliermarsh added the bug Something isn't working label Jul 12, 2024
@charliermarsh
Copy link
Member

I believe these are fixed by #12300.

charliermarsh added a commit that referenced this issue Jul 12, 2024
## Summary

I don't fully understand the purpose of this. In #7905, it was just
copied over from the previous non-preview implementation. But it means
that (e.g.) we don't treat `type(self.foo)` as a type -- which is wrong.

Closes #12290.
@timj
Copy link
Author

timj commented Jul 12, 2024

Can you give a concrete example of this?

Sorry. That was bad of me.

The code in question was https://github.com/lsst/ip_isr/blob/main/python/lsst/ip/isr/calibType.py#L139

elif type(attrSelf) != type(attrOther):

flake8 gave:

python/lsst/ip/isr/calibType.py:139:18: E721 do not compare types, for exact checks use `is` / `is not`, for instance checks use `isinstance()`

and ruff said nothing.

$ ruff --version
ruff 0.5.1
$ ruff check --select E721
All checks passed!

@dhruvmanila
Copy link
Member

@timj No worries! It's fixed and released in the latest version (0.5.2): https://play.ruff.rs/59525efe-9d7f-4683-83a9-808f15caacd6

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants