Consider making `NodeList` a generic over the type of widgets it holds

[rodrigogiraoserrao]

Right now, the implementation of _node_list.py::NodeList is a sequence of widgets.

If we make it a generic over the specific type of widgets it holds, we would get better type safety in ListView.

[willmcgugan]

The node list holds a widget’s children. Which means it can literally hold any widget type. I don’t think there is any room to make of generic.

[rodrigogiraoserrao]

The node list holds a widget’s children. Which means it can literally hold any widget type. I don’t think there is any room to make of generic.

The built-in type list also has the potential to hold any object and sometimes we have lists of ints or strings.
Similarly, ListView is always a container of ListItems. It isn't farfetched to assume that some containers in a user's app will have just a bunch of buttons, checkboxes, or some other custom widget.

That's what prompted this question & see the commit above that references this issue.

[davep]

The built-in type list also has the potential to hold any object and sometimes we have lists of ints or strings.

But those lists are normally created in a very specific context.

Given that NodeList is used for DOMNode._nodes, and that it's created in DOMNode.__init__, how and where do you see NodeList been told of any type other than Widget in normal use? Like, if someone wants to use a Vertical to only ever hold MyCoolWidget, how would you see the code working? And how would you then see things like mount being typed to work along with that (I've not dived in to look but I'd imagine it would have those sorts of knock-on effects)?

[willmcgugan]

It isn't farfetched to assume that some containers in a user's app will have just a bunch of buttons, checkboxes, or some other custom widget.

The thing is that a NodeList is used for one specific purpose, i.e. managing the DOM. The code that uses it only ever uses an interface exposed by Widget, i.e. it doesn't care if it is a Vertical or a Button.

The commit you referenced is probably not the best way forward. It will make the error go away, but the cast is invalid because we can't guarantee at runtime that it really will be a ListItem. This would still be true if we make NodeList a generic.

cast can be much the same as ignore. assert might be better, because that does guarantee that it will be true at runtime. Better still would be ensure that it is impossible for ListView to contain anything other than ListItems.

[davep]

See also RadioSet (earlier this week I was pondering the same issue as with ListView and ListItem).

Did make me wonder if there is or should be a on_child_mounted sort of event, to allow some checks - or something along those lines.

[rodrigogiraoserrao]

The commit you referenced is probably not the best way forward. It will make the error go away, but the cast is invalid because we can't guarantee at runtime that it really will be a ListItem. This would still be true if we make NodeList a generic.

Right, it doesn't ensure we have ListItems at runtime.
Neither does any of the static typing our code base has!
My understanding is that our codebase is typed so we can have a pre-flight sanity check.
I don't think we want to validate all the data types all the time during runtime.

If the users use ListView properly, then at that point we will have ListItem instances and I use the cast only to make mypy happy.

As for typing NodeList, I see that maybe it won't make a lot of sense.

[willmcgugan]

Right, it doesn't ensure we have ListItems at runtime. Neither does any of the static typing our code base has!

True, but the difference is that a developer can satisfy Mypy for this and it could still break. You can use the mount method to add anything to the ListView. In this case, Mypy is correct in complaining. An assert may be better, because we can catch the error early, rather than wait for it to blow up in unexpected ways.

A longer term solution may be to add some kind of mechanism to ensure only nodes of a given type may be added.

[rodrigogiraoserrao]

[...] True, but the difference is that a developer can satisfy Mypy for this and it could still break. [...]

Ahhh I see what you mean.
A developer that writes a type-safe program could still break this at runtime if it uses ListView.mount to add a non-ListItem to the list view. Is that it?

I was pondering on overriding mount and specifying that the widget to mount should be a ListItem but that would break Liskov's Substitution Principle!

[rodrigogiraoserrao]

Closing this for now as I think the discussion has reached a conclusion.

[github-actions]

Don't forget to star the repository!

Follow @textualizeio for Textual updates.