-
Notifications
You must be signed in to change notification settings - Fork 123
Refactor to use Enums consistently in PySTAC #231
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Comments
As mentioned in the linked issue, I'm a fan of using
Are you referring to code changes inside pystac or in third party libraries? I wouldn't expect many code changes, and if you're using existing classes as de-facto enums, you might not need any... If you have a class like class MediaType:
COG = 'image/tiff; application=geotiff; profile=cloud-optimized'
GEOJSON = 'application/geo+json'
GEOPACKAGE = 'application/geopackage+sqlite3' and you detect media type in code with if media_type == MediaType.COG then you need no changes there between the above and class MediaType(str, Enum): I'm guessing by backwards incompatible changes, you're referring to the string representation of the enum object? E.g. the current implementation prints the value of the enum print(MediaType.COG)
# image/tiff; application=geotiff; profile=cloud-optimized whereas changing to subclassing from print(MediaType.COG)
# MediaType.COG however this is easy to get around by overriding the class MediaType(str, Enum):
def __str__(self):
return str(self.value)
COG = 'image/tiff; application=geotiff; profile=cloud-optimized'
GEOJSON = 'application/geo+json'
GEOPACKAGE = 'application/geopackage+sqlite3'
print(MediaType.COG)
# image/tiff; application=geotiff; profile=cloud-optimized Any other things that you think wouldn't be backwards-compatible? |
@kylebarron this all sounds great, and I think it's the direction we should go in. In the PR you referenced you asked if a PR would be accepted - resounding yes! |
I'm on team enum. But there are definitely a few downsides to using them:
|
@geospatial-jeff good points - I was thinking about how some of the enum parameters should be annotated as part of #260, and didn't want to "block" passing in arbitrary strings for things like media types and link types that we don't have a complete list of. So I figured making parameters e.g. |
That is a good solution! Although I think you want to flip the order of the union so the less restrictive type comes first: |
Does the order of the Union matter? |
Currently, things like
CatalogType
,LinkType
etc are not enums but constants held in a class. In other places like the SAR extension, we use Enums.We should be consistent about our usage of enums across the library.
Note that moving from constants to enums would be a pretty big API break, potentially requiring a lot of client code changes. If we were to make that decision, it might be best to provide upgrade scripts to ease some of that API breakage pain.
If you have an opinion on whether or not Enums should be used across the library, please use this issue to voice it!
The text was updated successfully, but these errors were encountered: