Skip to content
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

pystac probably needs an implementation of StrEnum #265

Closed
schwehr opened this issue Feb 22, 2021 · 9 comments
Closed

pystac probably needs an implementation of StrEnum #265

schwehr opened this issue Feb 22, 2021 · 9 comments

Comments

@schwehr
Copy link
Collaborator

schwehr commented Feb 22, 2021

Just like the Enum package has enum.IntEnum, pystac should like have a StrEnum class. It doesn't have to be much, I since there are a bunch of them, it would be a bit cleaner.

class StrEnum(str, Enum):
    pass
find pystac -name \*.py | xargs grep Enum | grep str
pystac/extensions/__init__.py:class Extensions(str, Enum):
pystac/extensions/label.py:class LabelType(str, Enum):
pystac/link.py:class LinkType(str, Enum):
pystac/stac_object.py:class STACObjectType(str, Enum):
pystac/catalog.py:class CatalogType(str, Enum):
pystac/media_type.py:class MediaType(str, Enum):

After trying it a bit, I don't think it needs any extra code.

Hmmmm... StrEnum is in python 3.10, so, maybe not. Idonno.

@schwehr schwehr changed the title pystac probably needs an implementation of EnumStr pystac probably needs an implementation of StrEnum Feb 22, 2021
@schwehr
Copy link
Collaborator Author

schwehr commented Feb 22, 2021

@kylebarron
Copy link
Contributor

This was just changed #261. IMO making a new base class named StrEnum just to be able to remove class ClassName(str, Enum) doesn't make sense. Separate from that, it might not be a horrible idea to have a base Enum class that overrides __str__. I didn't do that at first because I thought there would only be a couple Enum classes, and then I found more 😄

@lossyrob
Copy link
Member

lossyrob commented May 4, 2021

I've found it confusing to have our Enums inherit from str, and in a few cases where the auto conversion was supposed to 'just work' I found I had to make explicit casts with str() to make contains checks work and make the type checker happy. E.g. I've found that "somestring" == Enum.SOME_STRING doesn't evaluate to True the way I was expecting, and requires explicit value conversion. So it seems like the transparent str|Enum approach is fraught, and I think we should prefer explicit .value calls when working with Enums to convert to the underlying value. I'm not sure if there's some advantage of also inheriting from str to ensure that .value is a str type, but overall I'm sour on implementing specific types of Enums and just using the base functionality and being explicit.

@kylebarron
Copy link
Contributor

kylebarron commented May 4, 2021

I've found that "somestring" == Enum.SOME_STRING doesn't evaluate to True the way I was expecting

Can you give an example of this? Here's a simple example using the same implementation of MediaType that compares to strings correctly:

from enum import Enum

class MediaType(str, Enum):
    def __str__(self):
        return str(self.value)

    PNG = 'image/png'

assert MediaType.PNG == 'image/png'

(Edit this is on Python 3.8.3)

@schwehr
Copy link
Collaborator Author

schwehr commented May 4, 2021

@kylebarron That works for my on python 3.6.

@lossyrob
Copy link
Member

lossyrob commented May 4, 2021

OK, after some investigation I think this is probably user error. I believe I saw the repr of a list of extensions where the Enum repr was in a list with strings, and blamed it on a separate issue. e.g. see below printing out a list of a mix of enums and strings gives some weird results, but the actual json.dump does the right thing.

Below is some ipython exploration that, along with your examples, convinces me that I was interpreting things incorrectly:

In [8]: class Example(str, enum.Enum):
   ...:     def __str__(self):
   ...:         return str(self.value)
   ...:
   ...:     ONE = 'one'
   ...:     TWO = 'two'
   ...:

In [9]: [Example.ONE]
Out[9]: [<Example.ONE: 'one'>]

In [10]: [Example.ONE, 'two']
Out[10]: [<Example.ONE: 'one'>, 'two']

In [11]: l = [Example.ONE, 'two']

In [12]: 'one' in l
Out[12]: True

In [13]: import json

In [14]: json.dumps(l)
Out[14]: '["one", "two"]'

In [15]: 'one' in json.loads(json.dumps(l))
Out[15]: True

@kylebarron
Copy link
Contributor

I believe I saw the repr of a list of extensions where the Enum repr was in a list with strings

Yeah I think letting Enum have a different repr than str is helpful because it lets you differentiate Enum values from str objects, but it can still be compared to a string.

@duckontheweb
Copy link
Contributor

As of #654 we have a StringEnum class that overrides the default __str__ implementation to return the string value of the enum. I briefly looked into using the Python 3.10 StrEnum with a backport, but it looks like backports.strenum only backports to Python >=3.8.6. Since we are still supporting Python 3.7 this doesn't seem like a good solution for now.

@schwehr @kylebarron It seems to me that our StringEnum implementation meets the needs described in this issue. Let me know if you disagree, otherwise I will close this issue.

@duckontheweb
Copy link
Contributor

Closed via #654

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants