don't type check __getattr__ #4616
Conversation
| @@ -137,6 +137,11 @@ Internal Changes | |||
| By `Yash Saboo <https://github.com/yashsaboo>`_, `Nirupam K N | |||
| <https://github.com/Nirupamkn>`_ and `Mathias Hauser | |||
| <https://github.com/mathause>`_. | |||
| - Added ``@no_type_check`` to ``__getattr__`` of :py:class:`Dataset` and :py:class:`DataArray`. | |||
There was a problem hiding this comment.
| - Added ``@no_type_check`` to ``__getattr__`` of :py:class:`Dataset` and :py:class:`DataArray`. | |
| - Added ``@no_type_check`` to ``__getattr__`` of :py:class:`Dataset` and :py:class:`DataArray`. |
| @@ -154,7 +154,8 @@ def _sum_of_weights( | |||
| """ Calculate the sum of weights, accounting for missing values """ | |||
|
|
|||
| # we need to mask data values that are nan; else the weights are wrong | |||
| mask = da.notnull() | |||
| # no type checking because ops are injected | |||
| mask = da.notnull() # type: ignore | |||
There was a problem hiding this comment.
Does this mean that any library downstream that runs .notnull will get a type error? I worry that'll cause a lot of false positives.
What do others think?
Yes you are right - I haven't thought this through! This does not only apply for |
|
In a way we have two mistakes cancelling each other out — missing methods and not type checking any method because of We could attempt to add in the methods explicitly rather than injecting them dynamically. I had thoughts about writing something to generate the python code & docstrings, which would give us the benefits of having them statically without the toil of writing them all out / keeping docstrings up to date, but not sure it would save enough time to make it worthwhile. Any other ideas / thoughts? |
|
Yes, I think that would be worthwhile. Not necessarily because of static typing but because it's quite difficult to understand. There is also a TODO on that: Lines 3 to 5 in 180e76d I think it should not be too difficult to create a Mixin class. Maybe the docstrings could still be injected. Not sure what is meant with |
|
I closed the PR by accident - no strong opinion whether to re-open it. Would be nice to get it in, but this can only be done once all the injected methods are replaced & I currently don't have time to work on this. |
isort . && black . && mypy . && flake8whats-new.rstIt's not pretty as I had to define a number of empty methods... I think this should wait for 0.17