Bring code coverage back up to 93-94%

[lossyrob]

The changes in #309 will cause the code coverage to dip to 88-89%. This issue is to ensure we bring back coverage to the 93-94% range for the PySTAC 1.0 release.

[duckontheweb]

#324 and #325 will probably go a long way towards this given the low coverage of both of those modules. We may want to wait to tackle this issue until those are complete and we have a better sense of the gap.

[duckontheweb]

We should also update the coverage badge link the main readme to point to the main branch instead of develop as part of this issue.

[l0b0]

I'd be interested in helping with this. I recently brought the coverage in another project from 97% to 100%, and it was surprising how many issues that uncovered (PR 1, 2, 3, 4).

[duckontheweb]

We would definitely appreciate the help on this!

One thing we may want to consider is whether we want to include testing of methods like __repr__ and __str__ in the coverage stats. I tend to feel like don't add much so I would be fine with excluding them, but I don't really feel strongly about it.

[l0b0]

One thing we may want to consider is whether we want to include testing of methods like __repr__ and __str__ in the coverage stats. I tend to feel like don't add much so I would be fine with excluding them, but I don't really feel strongly about it.

I feel a bit like something's wrong if none of the tests end up exercising these - usually that means they aren't needed and should be removed. But you can make a call on that once we get closer to 100%.

In a separate project we've ended up with the following excludes:

• if TYPE_CHECKING: since no tests run during type checking
• if __name__ == "__main__": since it can be assumed that this is run on the command line, and that the contents of this branch is tiny (typically just main())
• pragma: no cover to exclude a few pieces of test code which don't always run

[duckontheweb]

I feel a bit like something's wrong if none of the tests end up exercising these - usually that means they aren't needed and should be removed. But you can make a call on that once we get closer to 100%.

That's a good point. If they're important enough to add we should probably test them 😄 . We can see how it shakes out as we get closer.

[duckontheweb]

@l0b0 If you're okay with it, I'll assign this issue to you.

[l0b0]

I've got started on this work, and as usual when doing something like this, questions about the rationale behind any unusual code comes up. I noticed a bunch of the classes have custom __repr__ implementations, such as ItemSarExtension:

    def __repr__(self) -> str:
        return "<ItemSarExtension Item id={}>".format(self.item.id)

Based on the implementation above, repr(ItemSarExtension(first_item)) == repr(ItemSarExtension(second_item)) if and only if first_item.id == second_item.id, even if the other properties on first_item and second_item are different. As far as I can tell, there are two different schools of thought about which requirements the output of __repr__ should fulfil. One is that the output should be unique based on the properties of the object. That is, repr(ItemSarExtension(first_item)) == repr(ItemSarExtension(second_item)) should imply that repr(first_item) == repr(second_item) and repr(first_item.id) == repr(second_item.id) and so on for each of the properties of first_item and second_item, recursively. The second is that the output should contain enough information to recreate the object manually, basically making it a human readable representation of the object and all its properties, recursively. This implementation fulfils neither of the two, which could hide bugs when trying to work out for example why two seemingly identical objects behave differently.

In addition, I could only find a single place where repr() is used (in RequiredPropertyMissing), so I wonder if we could get rid of most or all of these.

I'll just skip adding coverage for __repr__ methods for now.

[l0b0]

Another question: Why is the orjson package optional? Presumably anyone using pystac will have to install some packages which are required, so why not make orjson mandatory and simplify the implementation, or revert to using the core json module?

Ping @duckontheweb?