Dirty workaround for mypy 1.5 error

[benbovy]

I wanted to fix the following error with mypy 1.5:

xarray/core/dataset.py:505: error: Definition of "__eq__" in base class "DatasetOpsMixin" is incompatible with definition in base class "Mapping"  [misc]

Which looks similar to python/mypy#9319. It is weird that here it worked with mypy versions < 1.5, though.

I don't know if there is a better fix, but I thought that redefining __eq__ in Dataset would be a bit less dirty workaround than adding type: ignore in the class declaration.

[Illviljan]

Here's an interesting read:

https://stackoverflow.com/questions/37557411/why-does-defining-the-argument-types-for-eq-throw-a-mypy-type-error

I can't check myself but does it work to just remove the Mapping base class?

[benbovy]

I found this SO post too, but I think that the issue here is rather specific to how mypy handles (mishandles?) multiple inheritance. I don't see any error reported using pyright. Also mypy doesn't complain anymore if I move the Mapping base class declaration to DatasetOpsMixin in order to remove multiple inheritance of __eq__:

# in xarray/core/_typed_ops.pyi

from abc import ABCMeta

class DatasetOpsMixin(
    Mapping[Hashable, "DataArray"],
    metaclass=ABCMeta,
):
    ...

# in xarray/core/dataset.py

class Dataset(
    DataWithCoords,
    DatasetAggregations,
    DatasetArithmetic,
):
    ...

But that is not a very nice solution either.

Simply removing the Mapping base class breaks a few things (key lookup, etc.) and removes important typing information.

[max-sixty]

Ah, this is a better version of #8155, sorry — I missed it.

If it's OK, I'll merge main in here and see whether that passes?

[max-sixty]

This passes the mypy check!

I've been out of the loop for a while, so don't want m return to be merging things I don't have much context for. But +1 on merging this, and will hit the button in a day or so unless someone objects.

[Illviljan]

Id prefer a solution without a type: ignore, they're too easy to add and aren't questioned enough.

But i dont have the time to look into it now either so no strong opinions here.

[max-sixty]

Id prefer a solution without a type: ignore, they're too easy to add and aren't questioned enough.

(I agree, but it doesn't look easy — such that holding the line against ignores may not inspire a better fix — but again I'm not sure)

[benbovy]

Let's merge this now so that we can have a few more green checks in main and in the updated PRs?

I asked upstream about the status of this issue. Hopefully this will be fixed at some point.

The workaround here looks reasonable to me, it doesn't add more type: ignore than there was before. Mypy still goes through all the Dataset class hierarchy.

[max-sixty]

We have green CI!