Some typing improvements #19
Conversation
This is far from working, just a small step
| @@ -159,7 +159,7 @@ def _getitem(self, select: ScippIndex) -> sc.DataArray: | |||
| continue | |||
| try: | |||
| sel = to_child_select(self.dims, field.dims, select) | |||
| coord = self[name][sel] | |||
| coord: sc.Variable = self[name][sel] | |||
There was a problem hiding this comment.
Not sure I see why this is required here (and other instances below). In what cases do we need to do this?
Do we also need it everywhere in scipp/neutron/ess?
There was a problem hiding this comment.
The problem (and the reason for many of the remaining errors that I did not manage (or care) to fix yet) is that __getitem__ may returns different things. Simplest example: h5py.Group.__getitem__ may give you a h5py.Group or a h5py.Dataset.
Do you know a solution?
There was a problem hiding this comment.
Is it limited to Group and Dataset or can it be something else?
If it's only those two, can you define what the __getitem__ will return at its definition? e.g.
def __getitem__(self, ...) -> Union[Group, Dataset]:I don't even know if that would help?..
There was a problem hiding this comment.
On the contrary, that is the source of the problem. If I now do anything with the return value I get complaints that the method may not exist, or follow-up errors about the type of the next __getitem__.
There was a problem hiding this comment.
Does this mean that every time we get an item from a python dict somewhere, we need to specify the type of what we are expecting?
There was a problem hiding this comment.
Good question, that is what we have to find out. Someone must have had this problem before us.
There was a problem hiding this comment.
Does this mean that every time we get an item from a python dict somewhere, we need to specify the type of what we are expecting?
Depends on how the dict is defined. If it is e.g. Dict[str, Union[int, str, tuple]], then yes, we would have to check every usage. And strictly speaking that makes sense because we don't know what we are getting. Usually we rely on Python's duck typing and getting TypeError or similar exceptions when the type does not match our expectations.
But typically, we would have a more concrete type in the dict: Dict[str, str] or any: Dict[str, Any] or just dict. In those cases, type checking is disabled.
There was a problem hiding this comment.
Is type checking something we want to strictly enforce everywhere in our codebase, or is it starting to get in the way of our development?
There was a problem hiding this comment.
It can catch bugs, so I feel we should try to use it more. Whether that means strictly enforcing it is something I cannot answer for now.
src/scippnexus/typing.py
Outdated
|
|
||
| Ellipsis = ellipsis.Ellipsis |
There was a problem hiding this comment.
Did you mean lowercase ellipsis? Right now, it conflicts the else branch.
There was a problem hiding this comment.
In what way? We need ellipsis to be a type (the class defined here). I got this from python/typing#684 (comment).
There was a problem hiding this comment.
Looks to me like Ellipsis is only defined in order to support if a is Ellipsis: in the example. In our case, writing 'Ellipsis' in code outside of typing.py would refer to the builtin type instead. So I am not sure the type checker would be happy with it.
In any case, I think line 66 can be removed in our case.
There are a lot of remaining mypy errors. Not sure what to do with most of them, for now.