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.

Sign up for GitHub

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

Jump to bottom

Remove enum prefixes #187

Merged
merged 47 commits into from
Oct 25, 2023
Merged

Remove enum prefixes #187

merged 47 commits into from
Oct 25, 2023

Conversation

Gobot1234
Copy link
Collaborator

@Gobot1234 Gobot1234 commented Dec 18, 2020 •
edited
Loading

Summary

Removes any enum class name prefixes from enum members.

NONE -> NONE
ARITHMETIC_OPERATOR_NONE -> NONE

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.
  • This PR adds something new (e.g. new method or parameters).
    • This change has an associated test
  • This PR is a breaking change (e.g. methods or parameters removed/renamed)
  • This PR is not a code change (e.g. documentation, README, ...)

Closes #174
Closes #302

Gobot1234 and others added 19 commits August 24, 2020 14:04
@Gobot1234
Implement Message.__bool__ for danielgtaylor#130
0d6ced5
@Gobot1234
Add a test for it
7746b91
@Gobot1234
Merge branch 'master' into master
53a7df0
@Gobot1234
Blacken
9da923d
@Gobot1234
Update tests
17e31f4
@Gobot1234
Fix bool
a53d805
@Gobot1234
Fix failing tests
b3b7c00
@Gobot1234
Update docs
4a4429d
@Gobot1234
Merge branch 'master' into master
48e80cf
@Gobot1234
Add __bool__ to special members
86d7c30
@Gobot1234
Update __init__.py
5c8e926
@Gobot1234
Simplify bool
f10bec4
@nat-n
Tweak __bool__ docstring
e04fcb6
@Gobot1234
Fixes for Python 3.9 (danielgtaylor#140)
5141d41 
Fix issue in logic for evaluating field types affecting python 3.9
@w4rum @Gobot1234
replace now-disabled set-env command (danielgtaylor#172)
b9a50a7 
thanks @abn
@abn @Gobot1234
Update dependencies and add ci checks for python 3.9 (danielgtaylor#173)
0869e69 
* Update locked dependencies to fix grpcio compile issue with python 3.9
* ci: add python 3.9
@abn @Gobot1234
Release v2.0.0b2 (danielgtaylor#175)
fe3addf
@nat-n @Gobot1234
Fix incorrect routes in generated client when service is not in a pac…
b82517d 
…kage (danielgtaylor#177)
@Gobot1234
Remove Enum prefixes
9fd8917
@Gobot1234 Gobot1234 changed the title Implement Message.__bool__ for #130 Remove enum prefixes Dec 18, 2020
@Gobot1234 Gobot1234 mentioned this pull request Dec 18, 2020
Closed
@Gobot1234
Copy link
Collaborator Author

I might actually just update this to remove any prefix on an enum that is the same for all the members.

nat-n
nat-n requested changes Jan 24, 2021
View reviewed changes
src/betterproto/compile/naming.py Outdated Show resolved Hide resolved
@nat-n nat-n mentioned this pull request Jan 25, 2021
Merged
@Gobot1234 Gobot1234 mentioned this pull request Jan 26, 2021
Closed
cetanu
cetanu previously approved these changes Dec 24, 2021
View reviewed changes
@Gobot1234 Gobot1234 mentioned this pull request Dec 29, 2021
Merged
3 tasks
@Gobot1234 Gobot1234 mentioned this pull request Mar 4, 2022
Merged
3 tasks
@Gobot1234
Merge branch 'master' into remove-enum-prefix
ce25f96 
# Conflicts:
#	src/betterproto/plugin/models.py
#	tests/inputs/enum/test_enum.py
@Gobot1234
Running tests is for nerds
992db51
@Gobot1234
Copy link
Collaborator Author

Failure looks unrelated

cetanu
cetanu approved these changes Apr 27, 2022
View reviewed changes
Copy link
Collaborator

@cetanu cetanu left a comment

Choose a reason for hiding this comment

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

I assume you will squash the commits for this 😅

@ryanwradtke
Copy link

Has this really been blocked for 2 years?

nat-n
nat-n previously requested changes Jul 16, 2022
View reviewed changes
Copy link
Collaborator

@nat-n nat-n left a comment

Choose a reason for hiding this comment

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

Hi, just dropping by again. You can get someone else to approve and merge this, but if you want my opinion it needs a little more thought.

src/betterproto/casing.py Show resolved Hide resolved
src/betterproto/compile/naming.py

def pythonize_enum_member_name(name: str, enum_name: str) -> str:
enum_name = casing.snake_case(enum_name).upper()
find = name.find(enum_name)
Copy link
Collaborator

@nat-n nat-n Jul 16, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I still think this implementation is unclear and error prone. For example imagine the chaos given an enum type with a single letter name! If the intention is to remove a "prefix", then just remove the prefix.

If you feel it's necessary to make it more clever than that (which I'm skeptical of) then a docstring explanation and test coverage are essential.

@Gobot1234 Gobot1234 requested a review from cetanu December 2, 2022 22:43
cetanu
cetanu previously approved these changes Oct 15, 2023
View reviewed changes
Copy link
Collaborator

@cetanu cetanu left a comment

Choose a reason for hiding this comment

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

I've looked over the files again, I am okay with this change. If some complication arises we can deal with it later.

@Gobot1234 Gobot1234 requested a review from cetanu October 16, 2023 11:55
@cetanu cetanu dismissed nat-n’s stale review October 16, 2023 22:17

Resolved comments due to lack of disagreement

@Gobot1234 Gobot1234 merged commit bd7de20 into danielgtaylor:master Oct 25, 2023
@Gobot1234 Gobot1234 deleted the remove-enum-prefix branch October 25, 2023 21:35
@snikch
Copy link

snikch commented Aug 4, 2024

FYI this has broken the ability to use enums in JSON payloads cross platform as the JSON value has been stripped of the prefix too.

From the linked C# docs in #174:

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

@snikch snikch mentioned this pull request Aug 4, 2024
Open
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nat-n nat-n nat-n left review comments

@cetanu cetanu cetanu approved these changes

Assignees
No one assigned
Labels
enhancement New feature or request high priority
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Enum with reserved word (None) clashes with python Enum Prefix is not stripped
7 participants
@Gobot1234 @cetanu @ryanwradtke @snikch @nat-n @w4rum @abn