Use enum for rel types and add staticmethod for canonical links

[duckontheweb]

Related Issue(s): #

#328

Description:

• Adds Link.canonical static method for creating links with a "canonical" rel type

  Since the "via" links can refer to arbitrary non-STAC records I didn't add an equivalent static method for those.

• Adds pystac.RelType enum with common rel values

  Any functions that accept a rel argument have been updated the type of the that parameter from str to Union[str, pystac.RelType] and I replaced any usages of common rel string literals with the corresponding enum values. I included all rel types that are introduced by extensions (label & version were the only ones) in the main pystac.RelType enum rather than putting them in their own enums to simplify things a bit. Also moved the rel types that were defined as constants in the Version Extension into this new enum.

• Uses pystac.MediaType enum values instead of string literals throughout the code

PR Checklist:

• Code is formatted (run scripts/format)
• Tests pass (run scripts/test)
• This PR maintains or improves overall codebase code coverage.
• Changes are added to the CHANGELOG. See the docs for information about adding to the changelog.

[duckontheweb]

@lossyrob Aside from the static methods, did you have any other changes in mind to support these rel types?

[codecov-commenter]

Codecov Report

Merging #351 (b8b2103) into main (61f62b1) will increase coverage by 0.05%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #351      +/-   ##
==========================================
+ Coverage   88.58%   88.64%   +0.05%     
==========================================
  Files          36       37       +1     
  Lines        4625     4648      +23     
==========================================
+ Hits         4097     4120      +23     
  Misses        528      528              

Impacted Files	Coverage Δ
pystac/serialization/common_properties.py	94.64% <ø> (ø)
pystac/serialization/identify.py	88.18% <ø> (ø)
pystac/__init__.py	97.14% <100.00%> (+0.08%)	⬆️
pystac/catalog.py	93.51% <100.00%> (ø)
pystac/collection.py	93.00% <100.00%> (ø)
pystac/extensions/hooks.py	86.44% <100.00%> (ø)
pystac/extensions/label.py	85.66% <100.00%> (ø)
pystac/extensions/version.py	95.74% <100.00%> (-0.18%)	⬇️
pystac/item.py	97.22% <100.00%> (ø)
pystac/link.py	93.98% <100.00%> (+0.13%)	⬆️
... and 4 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 61f62b1...b8b2103. Read the comment docs.

[lossyrob]

Hm, I actually misremembered and thought we had a Enum similar to MediaType that encapsulated common rel types. Now I see that we only have some of them included as static methods.

Perhaps we should a RelType enum? We'd could (similar to MediaType) change the signatures for e.g. the Link init to take a Union[str, RelType], as we want to indicate that a RelType would be prefered but not block other strings to be passed in.

The rel types would include any that are on the item, catalog or collection spec pages, as well as the best practices

Would this be worth it to add? I think it might help users decide what rel types to use for their links. If not then I'd say this PR is good to go

[duckontheweb]

Yeah, when I first read the issue I assumed there was an Enum as well. I'm +1 for adding a RelType enum and changing any callables that have a rel argument to accept Union[str, RelType]. This would also allow us to use checks like link.rel == RelType.ROOT instead of link.rel == "root", which is probably better practice.

I can add this to the PR and allow others to comment on that change in the review process.

[lossyrob]

Looks good, added some things to consider but nothing that I think is a blocker.

[lossyrob]

Perhaps we should link to the best practices docs here so users who want to learn more about how to use the reltypes have an easy find?

[duckontheweb]

Yeah, that's a good idea. I'll add a reference to those docs.

[lossyrob]

Including extension reltypes makes sense to me, though I could see a situation where someone is confused about using a reltype outside of the extension context or expecting to find the reltype in the extension. I'd say I'm +0 on this approach.

[duckontheweb]

To be honest, the more I sit with this approach the less I like it. I think it might be clearer to have rel types defined by extensions sitting with that extension code.

Here's an alternative proposal:

1. Create a base RelType enum:

   class RelType(str, Enum):
       def __str__(self) -> str:
           return str(self.value)

2. Have a CoreRelType enum that actually defines the values from the core spec and best practices:

   class CoreRelType(RelType):
       ALTERNATE = "alternate"
       ...

3. Extension define their own sub-class as needed:

   class LabelExtensionRelType(RelType):
       SOURCE = "source"

The type annotations for all the functions/classes that use rel types would stay as rel: Union[str, RelType] and extended rel types would be more clearly associated with the right extension. The downside would be that you wouldn't have a single place to enumerate all STAC rel types, you would need to specifically import something like pystac.extensions.label.LabelExtensionRelType, but maybe that explicit import is better anyway...

[lossyrob]

Yeah that makes sense to me. A little more work to use but a clearer organization.

I do think that having RelType hold all of the core rel types instead of making a separate CoreRelType would make sense; that would make LabelExtensionRelType also hold those core reltypes which is weird but it's probably more convenient to have the base RelType have the core instead of having to figure out there's a "core". Not sure though.

LabelExtension is getting a bit long, maybe LabelRelType or LabelExtRelType? Or perhaps it's best to have the full name for clarity, I usually don't mind additional typing if it makes code more consistent and clear. I do sometimes get accused of writing enterprise-y looking code though with long names 😆

[duckontheweb]

Python only allows you to subclass an enum with no members, so we wouldn't be able to have LabelExtRelType inherit from RelType if it included the core members.

It looks like we don't need to, though. Since it inherits from str, LabelExtRelType will pass a type check against str in any function arguments, so it doesn't need to inherit from RelType. As you mentioned in an earlier comment, we're really just using the Union[str, RelType] annotation to let users know they should use RelType if possible.

Given all of that, I would be for removing the extension-specific rel types from pystac.rel_type.RelType and putting them into their own enums in the extension modules.

[duckontheweb]

I implemented this in 2d53f0b so we can see how it feels. I'll revert it if we decide to go a different route.

[lossyrob]

Maybe it makes sense to add extension-specific relation types to a comment or docstring somewhere, to make it easier for users looking at the version source for the first time to see what RelTypes this extension adds? The constants-on-top approach the extensions have been nice for serving as a documentation of the spec - you see it adds these reltypes and you end up groking a bit more about the extension spec. Moving the extension rel types to RelType makes sense to me, as it keeps it all accessible in one place, and some source file docs would balance out the only negative effect I see of that change.

[duckontheweb]

@lossyrob Based on the discussion above I moved any rel types defined by extensions into the extension code. If you're okay with that approach I think we're ready to merge.

[lossyrob]

👍