-
Notifications
You must be signed in to change notification settings - Fork 107
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
Enable currently-ignored linting rules E721, E722, F403 #573
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## development #573 +/- ##
============================================
Coverage 65.07% 65.07%
============================================
Files 36 36
Lines 3052 3052
Branches 538 538
============================================
Hits 1986 1986
Misses 981 981
Partials 85 85 ☔ View full report in Codecov by Sentry. |
icepyx/tests/test_quest.py
Outdated
assert exp_key in obs.keys() | ||
assert type(obs[exp_key]) == exp_type | ||
assert isinstance(obs[exp_key], exp_type) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: this is a test so we have automatic confirmation it's fine, but for anyone not aware, there is a difference in behavior between type(...) == ...
and isintance(..., ...)
- isinstance
evaluates True
with inheritance:
>>> class Foo(str):
... ...
...
>>> type(Foo()) is str
False
>>> type(Foo()) is Foo
True
>>> isinstance(Foo(), str)
True
>>> isinstance(Foo(), Foo)
True
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. The ==
--> is
transition would be good enough to silence this ruff rule. The "comparing types" thing is frowned on in PEP8 and there is a different ruff rule that covers that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The more I think about it, the more I think we should go back to exact type comparison with type(...) is ...
. Since this is a test, loosening the comparison enables the underlying code to change more without breaking the test. We an always come back to this. My little mind is just thinking about the "foolish consistency" part of PEP8.
Would you mind?
Awesome, thank you! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apologies for all the mind-changing I'm doing today @cclauss. Could you address this before merge? https://github.com/icesat2py/icepyx/pull/573/files#r1722071874
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lovely! Thank you again 🙇
All three of these are discussed on PEP8 and should not be ignored.
E722 is https://realpython.com/the-most-diabolical-python-antipattern