-
Notifications
You must be signed in to change notification settings - Fork 129
Update pre-commit hooks #581
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
Conversation
I trust you there. Just have to remember to disable our dependency update bot then? |
@maresb wanna do it in this PR already? |
ea1617f
to
a66b277
Compare
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #581 +/- ##
==========================================
- Coverage 80.92% 80.92% -0.01%
==========================================
Files 162 162
Lines 46641 46648 +7
Branches 11399 11395 -4
==========================================
+ Hits 37746 37749 +3
- Misses 6667 6670 +3
- Partials 2228 2229 +1
|
@@ -689,7 +689,7 @@ def _lessbroken_deepcopy(a): | |||
else: | |||
rval = copy.deepcopy(a) | |||
|
|||
assert type(rval) == type(a), (type(rval), type(a)) | |||
assert type(rval) is type(a), (type(rval), type(a)) |
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.
Is this surely equivalent? Could the classes override type equality somehow?
The many places where we had type(...) == type(...)
make me weary of these changes.
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.
Ya, I am also weary of this commit. You should be very skeptical.
On my first attempt I also changed this line and it caused a test failure. (Note that this line is not like the others: .type
instead of type()
, and doesn't lead to a warning; I was just replacing ==
with is
wherever I saw it next to 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.
Yeah that's different. Is there a reference for this rule that we can look at? Maybe that will confirm it is indeed always safe to do so.
@ricardoV94, for a more sustainable solution I think we should install https://github.com/marketplace/pre-commit-ci
Description
Related Issue
Checklist
Type of change