basic typing fixes #1456
basic typing fixes #1456
Conversation
src/textual/_node_list.py
Outdated
| @@ -13,7 +14,7 @@ class DuplicateIds(Exception): | |||
|
|
|||
|
|
|||
| @rich.repr.auto(angular=True) | |||
| class NodeList(Sequence): | |||
| class NodeList(Sequence[Widget]): | |||
There was a problem hiding this comment.
Sequence is Generic and needs a type
There was a problem hiding this comment.
EDIT: importing Widget here causes cyclic dependency. Basically it makes DOM depend on Widget, and Widget itself obviously depends on DOM.
I resolved this by using T instead of Widget here. Apparently there's nothing in the code that needs an actual widget, only hashable things.
There was a problem hiding this comment.
Try putting Widget in quotes.
There was a problem hiding this comment.
I can do that, but isn't what I did now better? It really doesn't need to be Widget at all.
There was a problem hiding this comment.
The object is intended to be a container of Widgets. Better to type it as such.
There was a problem hiding this comment.
well in that case there's an actual cyclic dependency, no? I may be able to solve with quotes etc, but regardless there's a sort of "smell" to it. Why would DOMNode know about widgets? Anyway I'll have a look-see.
There was a problem hiding this comment.
Done. Works. (at least: passes mypy --strict and the tests...)
src/textual/renderables/sparkline.py
Outdated
|
|
||
| bucket_index = 0 | ||
| bars_rendered = 0 | ||
| step = len(buckets) / width | ||
| step = len(buckets) // width |
There was a problem hiding this comment.
NOTE -- I believe this is the intention, but not sure. Either way this failed strict typing, which is how I noticed.
| @@ -120,5 +121,5 @@ def frange(start, end, step): | |||
|
|
|||
| with Live(opacity_panel, refresh_per_second=60) as live: | |||
| for value in itertools.cycle(frange(0, 1, 0.05)): | |||
| opacity_panel.value = value | |||
| opacity_panel.opacity = value | |||
There was a problem hiding this comment.
This was an actual bug! opacity_panel does not have a value property, only opacity.
There was a problem hiding this comment.
This is dead code written to test the module. We should delete it.
There was a problem hiding this comment.
Will delete.
(EDIT: deleted; will appear as such when I push next)
src/textual/geometry.py
Outdated
| @@ -218,6 +219,8 @@ def contains_point(self, point: tuple[int, int]) -> bool: | |||
| def __contains__(self, other: Any) -> bool: | |||
| try: | |||
| x, y = other | |||
| assert isinstance(x, int) | |||
There was a problem hiding this comment.
I wouldn't consider the lack of a check to be a bug per se. Not averse to a sanity check, but it is an expensive one. Not sure it is worth the effort.
I'd also avoid raising an assert, just to catch it on the next line.
| @@ -63,6 +63,7 @@ def process_segments( | |||
| _from_color = Style.from_color | |||
| if opacity == 0: | |||
| for text, style, control in segments: | |||
| assert style is not None | |||
There was a problem hiding this comment.
Technically correct, but slow. There may be a lot of segments. Suggest ignoring or casting segments to say that style will not be None.
There was a problem hiding this comment.
I'll do the latter.
(EDIT: done; will appear as such when I push next)
There was a problem hiding this comment.
You can remove the assert now.
There was a problem hiding this comment.
Oops :) Removed.
| @@ -120,5 +121,5 @@ def frange(start, end, step): | |||
|
|
|||
| with Live(opacity_panel, refresh_per_second=60) as live: | |||
| for value in itertools.cycle(frange(0, 1, 0.05)): | |||
| opacity_panel.value = value | |||
| opacity_panel.opacity = value | |||
There was a problem hiding this comment.
This is dead code written to test the module. We should delete it.
|
@willmcgugan thank you for the review! I fixed everything save the |
| invisible_style = _from_color(bgcolor=style.bgcolor) | ||
| yield _Segment(cell_len(text) * " ", invisible_style) | ||
| else: | ||
| for segment in segments: | ||
| text, style, control = segment | ||
| if not style: | ||
| text, maybe_style, control = segment |
There was a problem hiding this comment.
I don't think it is a maybe style. It is a Style at this point. Styles do have __bool__ to indicate a null style, but it is still a Style object.
There was a problem hiding this comment.
It is still a maybe_style because in this branch I haven't cast segments to indicate it can't be None. I think you're saying I should do that in both branches. Will do.
|
@willmcgugan all comments addressed now, I believe. |
|
Well that's annoying. The tests pass perfectly other than one test in one configuration because... CI was too slow (test_auto_refresh). I propose to ignore that, or re-run. Also, I propose changing the measure. I might create a PR, I wonder if that works well in Windows etc. |
Hello there. I've run in a small number of typing issues in Textual during my everyday use, and thought I'd attempt a fix in Textual itself.
My ambitious goal is to run
mypy --stricton all the codebase with zero warnings, and I'm about 50% of the way though. However that would be a non-small PR, so I thought I'd start with some obvious issues that should be easy to accept hopefully.I've included here a fairly small number of changes, all of which I consider "obvious fixes" in the sense that they either expose actual bugs that exist now, or they break Python contracts, which are bugs waiting to happen.
I am going to annotate this PR with my reasoning below in a few minutes.
I hope this is useful. Cheers.