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

Add prefixed enums in addition to pretty ones #589

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

snikch
Copy link

@snikch snikch commented Aug 4, 2024

Summary

The representation of enums in JSON should not have the prefix removed as implemented in #187. This PR provides one option for fixing JSON representation of enums.

From the linked C# docs in #174 (referenced in the above PR):

This name transformation does not affect the text used within the JSON representation of messages.

This fix is a somewhat naive approach and I'm open to alternatives. It simply adds in the original enum field in addition to the stripped field.

I looked at an alternative where the JSON would render based on a full_name property, however the scope of this started to grow so I figured I'd push up the naive change and start a discussion.

One option would be to replace the value with a tuple of some sort, however it's possible this would create issues with comparisons.

class ArithmeticOperator(betterproto.Enum):
    """
    A "C" like enum with the enum name prefixed onto members, these should be stripped
    """

    NONE = (0, 'ARITHMETIC_OPERATOR_NONE')
    PLUS = (1, 'ARITHMETIC_OPERATOR_PLUS')
    MINUS = (2, 'ARITHMETIC_OPERATOR_MINUS')
    _0_PREFIXED = (3, 'ARITHMETIC_OPERATOR_0_PREFIXED')

Another option I thought of was to add an option as to whether to remove the prefix, however I personally would like to have the best of both words - nice python enums along with correct JSON encoding / decoding.

Thoughts?

Checklist

  • If code changes were made then they have been tested.
    • I have updated the documentation to reflect the changes.
  • This PR fixes an issue.

@Gobot1234
Copy link
Collaborator

Gobot1234 commented Aug 4, 2024

The full name version seems desirable. Could conditionally add the dict based on if it's necessary. The other options in my mind don't play nicely with type checking. See https://github.com/Gobot1234/steam.py/blob/main/steam/enums.py#L635 for how I've done it in my own projects might be useful

@snikch
Copy link
Author

snikch commented Aug 5, 2024

@Gobot1234 I went down a rabbit hole with custom enum types but ended up doing as you suggested. Updated!

Copy link
Collaborator

@Gobot1234 Gobot1234 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please can you also run poe fmt

Comment on lines +1496 to +1500
def name(enum_class, value):
obj = enum_class(value)
if hasattr(obj.__class__, 'full_name') and isinstance(obj.__class__.full_name, property):
return obj.full_name
return obj.name
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
def name(enum_class, value):
obj = enum_class(value)
if hasattr(obj.__class__, 'full_name') and isinstance(obj.__class__.full_name, property):
return obj.full_name
return obj.name
def name(enum_class, value):
obj = enum_class(value)
if hasattr(obj.__class__, 'full_name') and isinstance(obj.__class__.full_name, property):
return obj.full_name
return obj.name

This should probably go in utils or enum.py

Comment on lines +1580 to +1581
def obj(enum_class, value):
return enum_class.from_full_name(value) if hasattr(enum_class, 'from_full_name') else enum_class.from_string(value)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
def obj(enum_class, value):
return enum_class.from_full_name(value) if hasattr(enum_class, 'from_full_name') else enum_class.from_string(value)
def obj(enum_class, value):
return enum_class.from_full_name(value) if hasattr(enum_class, 'from_full_name') else enum_class.from_string(value)

Same as previous here

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

Successfully merging this pull request may close these issues.

2 participants