Fix selector hash function and the remainder of the types

[ocefpaf]

Adding proper types to ugrid.py revealed some inconsistencies that may require more testing in the future. Some variables are unused.

[ocefpaf]

@ChrisBarker-NOAA I'm not sure we do need a hash property/function here. Still, I "fixed" what seemed to be a placeholder by using the most unique property for hashing I could imagine for this class.

[ChrisBarker-NOAA]

IIUC, setting __hash__ is not a placeholder, but rather makes the type non-hashable -- I'm not sure why that was done, but it was presumably deliberate, and I think it's better for it to be non-hashable than have a hash that may not work correctly.

I think we should put the None back until / if we figure out why you might want a hash for this.

[ocefpaf]

Then we can do:

    def __hash__(self):
        return hash(tuple(self))

to make it unhasable.

[ChrisBarker-NOAA]

what's the advantage of that over hash = None?

I think __hash__ = None is the conical way to do it.

"""
When the __hash__() method of a class is None, instances of the class will raise an appropriate TypeError when a program attempts to retrieve their hash value, and will also be correctly identified as unhashable
"""

[ocefpaf]

Actually, if I understood it correctly, to make something unhashble all we need to do is to not define __hash__. This happens in the latest commit, is that what you expect here?

from xarray_subset_grid.selector import Selector ; a = Selector() ; hash(a)

TypeError: unhashable type: 'Selector'

[ocefpaf]

I don't know the pattern __hash__ = None, and mypy treats that like an error b/c we cannot define a hash that is unhashable.

After some research I think that not defining is the way to make it unhashable.

[ChrisBarker-NOAA]

If MyPy is confused, it's MyPy's problem -- I don't think we should ever change correct code to satisfy typing.

(but I'm a typing skeptic)

If __hash__ is not defined, then it uses the id as the default hash.

[ocefpaf]

@ChrisBarker-NOAA this one is ready for review.

[ChrisBarker-NOAA]

other than the hash question this LGTM.

thanks!

[ocefpaf]

other than the hash question this LGTM.

Are you OK with just removing the __hash__ to make it unhashable? It would also satisfy mypy that way.

[ChrisBarker-NOAA]

Looking at this a bit more:

"If your class overrides the __eq__ method, Python automatically sets __hash__ = None under the hood at runtime."

In this case, __eq__ is set, so that should be fine.

But I still prefer the explicit -- "this class is not hashable" approach, and we can satisfy MyPy with:

    __hash__ = None  # type: ignore[assignment]

or maybe:

_hash__: Callable | None

But how to specify Callable[self] ?

And this why I'm a typing skeptic ...

And I really don't know MyPy, so your choice.

[ocefpaf]

And this why I'm a typing skeptic ...

I agree. It does help, but sometimes we are just trying to satisfy a "linter".

It does shine for large codebases where these the value/idiosyncrasies ratio is higher.

I'll make a explicit None and add the mypy skip tomorrow.