issue_comments

@nbren12 it seems to me that mypy is being overly aggressive when parsing the hinted code (hence why I had to put # type: ignore on it) but it is being more lax when the same code is invoked somewhere else like in my test script. Overall I suspect it may be fragile and break in future mypy versions...

@crusaderky Thanks for the re-work. For my own benefit, could you explain why that code worked? I remember writing something very similar, and running into mypy errors. My understanding of how mypy intreprets overload seems incomplete.

I took the liberty to rework it, please have a look Test script:

```python from typing import Hashable, Mapping import xarray ds: xarray.Dataset

class D(Hashable, Mapping): def hash(self): ... def getitem(self, item): ... def iter(self): ... def len(self): ...

reveal_type(ds["foo"]) reveal_type(ds[["foo", "bar"]]) reveal_type(ds[{}]) reveal_type(ds[D()]) mypy output: t1.py:12: note: Revealed type is 'xarray.core.dataarray.DataArray' t1.py:13: note: Revealed type is 'xarray.core.dataset.Dataset' t1.py:14: note: Revealed type is 'xarray.core.dataset.Dataset' t1.py:15: note: Revealed type is 'xarray.core.dataset.Dataset' ```

Hello @nbren12! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! :beers:

Comment last updated at 2020-06-13 17:43:56 UTC

No problem! I think I am done with this one unless you think its important that I document or test this somehow. Can someone review it?

Nice find @mathause ; I remember that discussion now.

As @nbren12 said, this doesn't go all the way given mypy's restrictions, but seems like a dominant improvement.

Test failures are unrelated.

Thanks @nbren12 !

Seems this was already discussed in GH3210 (comment) and see also the TODO:

https://github.com/pydata/xarray/blob/8f688ea92ae8416ecc3e18f6e060dad16960e9ac/xarray/core/dataset.py#L1244-L1250

(although it is not entirely clear to me whether this is actually fixed or not)

Okay. Assuming the tests pass, I think this is ready for review. I tried adding a test, but mypy didn't seem to find problems even with code that I know doesn't work (e.g. 'a'+ 1). Is there some strategy for testing tricky type hints like this?

In any case, this code does work: ``` $ cat test_mypy.py (fv3net) import xarray as xr ds = xr.Dataset({"a": ()})

arr = ds['a'] union_obj = ds[['a']]

reveal_locals() $ mypy test_mypy.py (fv3net) test_mypy.py:8: note: Revealed local types are: test_mypy.py:8: note: arr: xarray.core.dataarray.DataArray test_mypy.py:8: note: ds: xarray.core.dataset.Dataset test_mypy.py:8: note: union_obj: Union[xarray.core.dataarray.DataArray, xarray.core.dataset.Dataset] ```

@mathause On further consideration, I think it might not be possible to get this to work. This method has three behaviors: - Mapping -> Dataset - Hashable -> DataArray - else (List): -> Dataset

With my limited understanding of mypy, I think that any two of these is supported by overload, but I'm not sure it's possible to support all 3. I tried several different options, but maybe I am missing something.

Would a good middle ground be something like this? - Hashable -> DataArray - Any -> Union[DataArray, Dataset]

I think this would work since both the input/outputs of the first one are subtypes of the second one. It's not a complete solution, but it would solve the most common problem of ds['a'] returning a union type rather than a DataArray.

The mypy check throws an error: xarray/core/dataset.py:1250: error: Overloaded function signature 2 will never be matched: signature 1's parameter type(s) are the same I think you can ignore the other failures. Should that be:

```python @overload def getitem(self, key: Hashable) -> DataArray: ...

@overload
def __getitem__(self, key: Iterable[Hashable]) -> "Dataset":
    ...

`` ? Only guessing, though - it wasAny` so there may be more options. @crusaderky