enable the mypy "truthy-bool" error code - #9409
Conversation
Sorry, something went wrong.
|
|
||
| try: | ||
| from . import BmpImagePlugin | ||
| from . import BmpImagePlugin as BmpImagePlugin |
There was a problem hiding this comment.
This might look a bit weird (unless you're used to writing type stubs), but it avoids a # noqa. I wouldn't mind using # noqa here instead if that's preferred
Sorry, something went wrong.
| pass | ||
| else: | ||
| if image and image.mode in ("1", "L"): | ||
| if image.mode in ("1", "L"): |
There was a problem hiding this comment.
at this point image cannot be None anymore
Sorry, something went wrong.
|
|
||
| reloaded = Image.fromarrow(arr, mode, img.size) | ||
|
|
||
| assert reloaded |
There was a problem hiding this comment.
I could also change this to an assert isinstance or something, but I figured that'd also be kinda obvious.
Sorry, something went wrong.
| # Segfault test | ||
| app: QApplication | None = QApplication([]) | ||
| ex = Example() | ||
| ex = Example() # noqa: F841 |
There was a problem hiding this comment.
I'm not sure what the idea here is, but it looked as if the assert was purely there to avoid this F841 error.
Sorry, something went wrong.
There was a problem hiding this comment.
Is it not preferable to avoid noqa where possible? My solution would have been assert ex is not None
Sorry, something went wrong.
There was a problem hiding this comment.
Personally I prefer being explicit about ignored errors. And assert statements aren't "free" (the runtime impact is probably negligible, but still).
And, if at some point, some change causes this so that ex could actually become None, then ruff will complain about an unused # noqa, which would prevent a potential bug.
Sorry, something went wrong.
hugovk
left a comment
There was a problem hiding this comment.
I don't think these changes do much to improve the readability of the code.
Some of the existing hints make core less readable and were to silence mypy. Pillow is an old code base (31 years old!), so I think we need to careful about introducing more strictness.
This is one of the additional checks that are required by repo-review (MY106): learn.scientific-python.org/development/guides/style#MY106 IMO it's more of a linting rule than a type-checking rule, but either way, I think it's a pretty decent one to enable. FWIW; In the projects I maintain (numpy, scipy-stubs, ...), I always enable it.
That link says:
The extra strict options shown above, like warn_unreachable MY103, and ignore-without-code MY104, redundant-expr MY105, and truthy-bool MY106 can trigger too often (like on sys.platform checks) and have to be ignored occasionally, but can find some significant logic errors in your typing.
It seems to be triggering too often in this case, let's skip this one.
Sorry, something went wrong.
|
Seems fair; closing |
Sorry, something went wrong.
This is one of the additional checks that are required by repo-review (MY106): https://learn.scientific-python.org/development/guides/style/#MY106
IMO it's more of a linting rule than a type-checking rule, but either way, I think it's a pretty decent one to enable. FWIW; In the projects I maintain (numpy, scipy-stubs, ...), I always enable it.
mypy docs: https://mypy.readthedocs.io/en/stable/error_code_list2.html#check-that-expression-is-not-implicitly-true-in-boolean-context-truthy-bool