fix(types): typing fixes exposed by extra checking #2131
Conversation
Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com>
Codecov Report
@@ Coverage Diff @@
## master #2131 +/- ##
==========================================
- Coverage 99.46% 99.07% -0.40%
==========================================
Files 72 72
Lines 7339 7555 +216
==========================================
+ Hits 7300 7485 +185
- Misses 39 70 +31
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
rich/spinner.py
Outdated
| # This normally can't be str, unless someone assigned it later. | ||
| if not self.text: # type: ignore[truthy-bool] |
There was a problem hiding this comment.
Typewise this can't happen, but I could see it possibly happing at runtime, so just added an ignore and note here.
There was a problem hiding this comment.
self.text may be a Text instance, which has a __bool__
I suspect the root cause of the issue is that text in the constructor is typed as a RenderableType, where it should probably be a Union[Text, str].
There was a problem hiding this comment.
Above, in the constructor, self.text = Text.from_markup(text) if isinstance(text, str) else text is set. This can't be a str, but it could be Union[Text, RenderableType], perhaps? I'd rather have thought it would already have been inferred to have that type, actually, but maybe not due to Text fitting into the RenderableType protocol. I'll see if I can try that.
There was a problem hiding this comment.
Yes, that appears to be it, mypy assigns self.text: RenderableType because Text fits that Protocol. But the __bool__ behavior is unique to Text, so this confuses it below. Making this explicit solves the issue.
| @@ -740,7 +740,7 @@ def __add__(self, style: Optional["Style"]) -> "Style": | |||
| new_style._link = style._link or self._link | |||
| new_style._link_id = style._link_id or self._link_id | |||
| new_style._hash = style._hash | |||
| new_style._null = self._null or style._null | |||
| new_style._null = style._null | |||
There was a problem hiding this comment.
There's a guard about 10 lines above:
if self._null:
return styleSo self._null cannot be truthy at this point.
| @@ -54,10 +54,9 @@ build-backend = "poetry.core.masonry.api" | |||
|
|
|||
| [tool.mypy] | |||
| files = ["rich"] | |||
| warn_unused_configs = true | |||
There was a problem hiding this comment.
This is included in strict, which I didn't realize until checking the mypy --help output just now.
Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com>
henryiii
force-pushed
the
henryiii/chore/mypy7
branch
from
62b58d5 to
46b1c95
Compare
|
Thanks |
Type of changes
Checklist
Description
This enables a few extra mypy checks, and fixes type bugs exposed by those. There were three features enabled, but I've left one off:
warn_unrachable = true. While it is really useful, and helped expose a couple of the issues fixed here, it also requires type ignoring in about 5-7 places unavoidably, so I've left it off. The most irritating one isif sys.platform.startswith("win")constructs, which it will notice are unreachable if you are typing targeting a different platform. MyPy might fix this in python/mypy#12286, which would run all platforms/versions in parallel and then combine the results (which would be really fantastic!), but until then, it's likely better just to manually add--warn-unreachableonce in a while and make sure the errors reported are expected. (Depending on your preference, of course - I've been turning it on mostly, but I usually don't have quite as many ignores due to it - I thik I counted at least 7 here. If you want to see it, let me know, and I'll PR it).I'll comment inline on the others, but this is one fix:
is invalid. You can't take the length of an iterator. Someone could correctly put an iterator function in here, and this will break. Either this should be
Sequence(which is what I've used here), or you should iterate over the iterator and build a sequence like a list or a tuple and then use that.