Equality of pystac objects?

[TomAugspurger]

In writing tests for #379, I stumbled over Asset objects not defining __eq__.

In [1]: import pystac

In [2]: pystac.Asset(href='a') == pystac.Asset(href='a')
Out[2]: False

Looking briefly, I don't see it for any of the core items (a couple extensions define __eq__). The behavior of Python is to fall back to object identity, (a is b).

Should pystac objects define __eq__ (and __ne__)? An implementation like the following probably makes sense:

def __eq__(self, other):
    return type(self) == type(other) and self.to_dict() == other.to_dict()

If we define __eq__ then we may need to make the objects unhashable, since the objects are mutable. https://hynek.me/articles/hashes-and-equality/ is a good read on this.

[duckontheweb]

I'm in favor of defining equality for PySTAC objects.

As I started working on an ItemCollection class for #371 I realized it would be nice to be able to define __contains__ on that class to check if an ItemCollection contains an Item. This is currently possible because I am cloning each Item during ItemCollection initialization so that we can reset the root link without mutating the original object. Since the default equality measure is item_a is item_b, as @TomAugspurger mentioned, this always comes back False.

I can tackle this as part of the work on #371. It sounds like we should raise a NotImplementedError for values of other that cannot be compared, so I'm thinking this should be the implementation:

def __eq__(self, other: object) -> bool:
    if type(self) != type(other):
        raise NotImplementedError(f"Type of other must be {type(self)}"
    return self.to_dict() == other.to_dict()

def __ne__(self, other: object) -> bool:
    if type(self) != type(other):
        raise NotImplementedError(f"Type of other must be {type(self)}"
    return self.to_dict() != other.to_dict()

That article mentions that Python 3 automatically makes objects unhashable if you define __eq__ and not __hash__, but I'll double-check that.

[TomAugspurger]

It sounds like we should raise a NotImplementedError for values of other that cannot be compared, so I'm thinking this should be the implementation:

I haven't read closely, but I want to confirm: do you want raise NotImplementedError or return NotImplemented. The (subtle) difference is that return NotImplemented allows the other object to determine if it's equal or not. I think if both return NotImplemented then Python raises a TypeError.

How to handle "foreign" objects is a really tricky subject, but I think the best practice is to only consider your own types and to return NotImplemented for all the others.

[duckontheweb]

do you want raise NotImplementedError or return NotImplemented.

Yes, thanks for pointing that out, I misread that. We want to return NotImplemented if it's "foreign".

[duckontheweb]

It might be worth a little more discussion on how we want to specifically define equality. I'm realizing now that the above definition won't necessarily work in an ItemCollection since we are removing any root links for Items when we initialize the ItemCollection, which makes the dicts unequal.

It seems like there could be a use-case for considering otherwise identical Items that belong to different Catalogs or Collections to be equal. In that case, if the "collection" property and the "parent", "root", and "collection" links were different, but all other properties were the same the Items would be considered equal. There might be a similar use-case for Collections that are included in multiple APIs or Catalogs.

[duckontheweb]

cc: @gadomski