issue_comments

Big win @dcherian ! Thanks a lot. By LoC, xarray just got 4% better in a single PR!

Thanks for the reviews!

This can be merged as is. I just think it looks a little neater with |.

:+1: I think we make this change project-wide if we want to do it (looks like pyupgrade supports it). @Illviljan can you open an issue please?

from __future__ import annotations does not actually directly enable the use of | for union, it changes the semantics of type annotations to postpone evaluation. In particular, you can then put just about anything in the type annotation without having any runtime behavior

pandas uses | and from __future__ import annotations quite frequently in order to avoid the Union, for example: https://github.com/pandas-dev/pandas/blob/master/pandas/plotting/_core.py

Having typing not doing any runtime calculations sounds great to me. We usually don't need the typing variables during runtime anyway.

This can be merged as is. I just think it looks a little neater with |.

This looks great, nice work!

This can be saved for a future PR but one thing I notice is that it's possible to make the unions slightly more readable using the new |-operator: python dim: Union[None, Hashable, Sequence[Hashable]] = None vs. ```python from future import annotations

dim: None | Hashable | Sequence[Hashable] = None ``` I'm unsure about the compatibility but I think it should be fine.

All green! This should be good to go.

Here are a couple of examples: https://xray--5871.org.readthedocs.build/en/5871/generated/xarray.core.groupby.DataArrayGroupBy.sum.html

https://xray--5871.org.readthedocs.build/en/5871/generated/xarray.core.resample.DataArrayResample.sum.html

Unless @shoyer has any objections — I think we were primarily delayed by Apache Beam, and they have unlocked that dependency, so I would vote to add it as a required dependency. The only realistic way this could be bad is if there's another package that is commonly installed with xarray that a) requires typing_extensions, b) forbids a recent version, c) won't remove that pin soon.

+1 for adding a requirement on typing_extensions. It seems a little early to be dropping Python 3.7.

Unless @shoyer has any objections — I think we were primarily delayed by Apache Beam, and they have unlocked that dependency, so I would vote to add it as a required dependency. The only realistic way this could be bad is if there's another package that is commonly installed with xarray that a) requires typing_extensions, b) forbids a recent version, c) won't remove that pin soon.

It's ok to me. My hesitation is because we only need in a very small number of places IIUC.

Happy to defer to the opinions of more experienced typers

I think @keewis mentioned we could drop 3.7 in December which is not that far away. Can we type self as Any for now and update later?

What are your thoughts re adding the typing_extensions dependency?

I think @keewis mentioned we could drop 3.7 in December which is not that far away. Can we type self as Any for now and update later?

I just checked Apache beam (the library that prevented us upgrading last time), and they now support typing-extensions<4, so I would vote to add that dependency, rather than drop 3.7.

We can probably do the workaround way like in #5624: https://github.com/pydata/xarray/blob/b79155862e154db45dff3b526c2f9e8ef303808f/xarray/core/options.py#L7-L72

But I find typing_extensions getting rather important, most of the time when you google for the typing issue the proper solutions often uses these new typing additions.

Bah, Protocol isn't supported in 3.7! We could either revisit mandating typing_extensions or drop 3.7 (which is overdue since our change to only allow 24 months for python releases).

Is it possible to mandate typing_extensions for only 3.7?

I fixed mypy! @Illviljan thanks for the Protocol suggestion. I had to make a different one per class, because the return type for reduce couldn't be generic (I think, unless there's a way...)

Actually much more tedious was fixing black given line-breaks etc.

Great work @dcherian !

I'm having trouble with mypy though. Please help!

I tried for a while. I'm still trying!

For the typing issues you can try out using Protocol: https://stackoverflow.com/questions/51930339/how-do-i-correctly-add-type-hints-to-mixin-classes

Updated to generate resample reductions separately. This allows us to have nice examples in the docstring.

I'm having trouble with mypy though. Please help! xarray/core/_reductions.py:2662: error: "DataArrayResampleReductions" has no attribute "reduce" [attr-defined] xarray/core/_reductions.py:2737: error: "DataArrayResampleReductions" has no attribute "reduce" [attr-defined] xarray/core/_reductions.py:2805: error: "DataArrayResampleReductions" has no attribute "reduce" [attr-defined] xarray/core/_reductions.py:2872: error: "DataArrayResampleReductions" has no attribute "reduce" [attr-defined] xarray/core/_reductions.py:2939: error: "DataArrayResampleReductions" has no attribute "reduce" [attr-defined]