Add prefixed enums in addition to pretty ones#589
Open
snikch wants to merge 10 commits intodanielgtaylor:masterfrom
Open
Add prefixed enums in addition to pretty ones#589snikch wants to merge 10 commits intodanielgtaylor:masterfrom
snikch wants to merge 10 commits intodanielgtaylor:masterfrom
Conversation
Collaborator
|
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 |
Author
|
@Gobot1234 I went down a rabbit hole with custom enum types but ended up doing as you suggested. Updated! |
Gobot1234
approved these changes
Aug 14, 2024
Collaborator
Gobot1234
left a comment
There was a problem hiding this comment.
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 |
Collaborator
There was a problem hiding this comment.
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) |
Collaborator
There was a problem hiding this comment.
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
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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 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_nameproperty, 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.
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