-
Notifications
You must be signed in to change notification settings - Fork 25
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
Repeatedly double-check return values of zero when spying on super_len()
#1370
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #1370 +/- ##
==========================================
- Coverage 88.73% 88.54% -0.20%
==========================================
Files 77 77
Lines 10424 10460 +36
==========================================
+ Hits 9250 9262 +12
- Misses 1174 1198 +24
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
if ( | ||
n == 0 | ||
and isinstance(o, io.IOBase) | ||
and (name := getattr(o, "name", None)) not in (None, "") |
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.
Previously we didn't have those checks of io and name, would we add at least a debug message if we get 0 for those cases?
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.
Previously we didn't have those checks of io and name
Previously, we weren't checking for anything.
would we add at least a debug message if we get 0 for those cases?
I can't tell what you're trying to say here.
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.
I thought that we had some dedicated debug message for size 0 only but now I see that it was for all sizes...
My point was that what if we still get super_len to report 0 but then it would not be matching those two other conditions and thus we silently would not be testing using check_len
and it might not be clear from the debug message above on either we should have or should have not... but I also see that we should be reporting some more debug lines from within check_len so we could tell between those cases anyways, so probably no more logging is needed, let's proceed.
🚀 PR was released in |
Part of #1257.