8000 Remove enum prefixes by Gobot1234 · Pull Request #187 · danielgtaylor/python-betterproto · GitHub
[go: up one dir, main page]

Skip to content

Remove enum prefixes #187

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

Merged
merged 47 commits into from
Oct 25, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
47 commits
Select commit Hold shift + click to select a range
0d6ced5
Implement Message.__bool__ for #130
Gobot1234 Aug 24, 2020
7746b91
Add a test for it
Gobot1234 Aug 24, 2020
53a7df0
Merge branch 'master' into master
Gobot1234 Sep 2, 2020
9da923d
Blacken
Gobot1234 Sep 2, 2020
17e31f4
Update tests
Gobot1234 Sep 19, 2020
a53d805
Fix bool
Gobot1234 Sep 19, 2020
b3b7c00
Fix failing tests
Gobot1234 Sep 19, 2020
4a4429d
Update docs
Gobot1234 Oct 19, 2020
48e80cf
Merge branch 'master' into master
Gobot1234 Oct 19, 2020
86d7c30
Add __bool__ to special members
Gobot1234 Oct 20, 2020
5c8e926
Update __init__.py
Gobot1234 Oct 20, 2020
f10bec4
Simplify bool
Gobot1234 Oct 27, 2020
e04fcb6
Tweak __bool__ docstring
nat-n Nov 24, 2020
5141d41
Fixes for Python 3.9 (#140)
Gobot1234 Nov 1, 2020
b9a50a7
replace now-disabled set-env command (#172)
w4rum Nov 21, 2020
0869e69
Update dependencies and add ci checks for python 3.9 (#173)
abn Nov 24, 2020
fe3addf
Release v2.0.0b2 (#175)
abn Nov 24, 2020
8000
b82517d
Fix incorrect routes in generated client when service is not in a pac…
nat-n Nov 28, 2020
9fd8917
Remove Enum prefixes
Gobot1234 Dec 18, 2020
264c4b9
Merge remote-tracking branch 'upstream/master' into remove-enum-prefix
Gobot1234 Dec 18, 2020
d965741
Blacken
Gobot1234 Dec 18, 2020
9b9f737
Remove any repeated prefix from enum members
Gobot1234 Dec 30, 2020
60d1880
Remove unused imports
Gobot1234 Dec 30, 2020
ae80862
That was a pain
Gobot1234 Dec 30, 2020
5adc93d
Revert stuff
Gobot1234 Jan 26, 2021
a7df010
Fix stuff
Gobot1234 Feb 1, 2021
d58ce63
Relock
Gobot1234 Apr 2, 2021
17ba889
Merge remote-tracking branch 'upstream/master' into remove-enum-prefix
Gobot1234 Apr 2, 2021
b1ba675
Relock?
Gobot1234 Apr 2, 2021
e2b850e
Update test_enum.py
Gobot1234 Apr 2, 2021
bbe7256
Update naming.py
Gobot1234 Apr 2, 2021
ecc1629
Relock
Gobot1234 Dec 24, 2021
7b6f03d
Merge branch 'remove-enum-prefix' of https://github.com/Gobot1234/pyt…
Gobot1234 Dec 24, 2021
6592d4d
Merge branch 'master' into remove-enum-prefix
Gobot1234 Dec 24, 2021
6fe9b5c
Lock again
Gobot1234 Dec 24, 2021
3afffca
Fix for names starting with ints
Gobot1234 Dec 24, 2021
52456c7
Update the lock file
Gobot1234 Apr 20, 2022
ce25f96
Merge branch 'master' into remove-enum-prefix
Gobot1234 Apr 20, 2022
992db51
Running tests is for nerds
Gobot1234 Apr 20, 2022
72a8b89
Add a test for the invalid identifier case
Gobot1234 Apr 21, 2022
a2ba23e
Update casing.py
Gobot1234 Apr 21, 2022
25ef492
Merge branch 'master' into remove-enum-prefix
Gobot1234 Oct 16, 2023
383575f
Check for dunder names
Gobot1234 Oct 16, 2023
ecae7f6
Rejig order
Gobot1234 Oct 16, 2023
0cc1790
Shim get source
Gobot1234 Oct 16, 2023
6392a9d
Remove redundant conditional
Gobot1234 Oct 16, 2023
a70a00d
Remove source shim
Gobot1234 Oct 16, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion src/betterproto/casing.py
Original file line number Diff line number Diff line change
Expand Up @@ -136,4 +136,8 @@ def lowercase_first(value: str) -> str:

def sanitize_name(value: str) -> str:
# https://www.python.org/dev/peps/pep-0008/#descriptive-naming-styles
return f"{value}_" if keyword.iskeyword(value) else value
if keyword.iskeyword(value):
return f"{value}_"
if not value.isidentifier():
return f"_{value}"
return value
8 changes: 8 additions & 0 deletions src/betterproto/compile/naming.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,3 +11,11 @@ def pythonize_field_name(name: str) -> str:

def pythonize_method_name(name: str) -> str:
return casing.safe_snake_case(name)


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

Choose a reason for hiding this comment

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

I don't think removing the enum name from just anywhere in the key name would be a good idea, especially if each key is evaluated independently.

Suggested change
find = name.find(enum_name)
prefix = casing.snake_case(enum_name).upper() + "_"
if name.startswith(prefix):
return name[len(prefix):]
return name

I wonder if it's better to be all or nothing here... i.e. only do the renaming if the convention is perfectly followed (by all keys). Do you know of a precedent or documented rule to follow one way or the other?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I used find because sometimes people in C use k_ to prefix an enum.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Why would a prefix of k_ necessitate str.find? Just curious

Do we just need a small test for this case (and any others)?

Copy link
Collaborator Author
@Gobot1234 Gobot1234 Mar 17, 2021

Choose a reason for hiding this comment

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

Because the enum name doesn't start with k_

Copy link
Collaborator
@nat-n nat-n Jul 16, 2022

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.

if find != -1:
name = name[find + len(enum_name) :].strip("_")
return casing.sanitize_name(name)
5 changes: 1 addition & 4 deletions src/betterproto/enum.py
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ def __new__(
members = {
name: value
for name, value in namespace.items()
if not _is_descriptor(value) and name[0] != "_"
if not _is_descriptor(value) and not name.startswith("__")
}

cls = type.__new__(
Expand All @@ -70,9 +70,6 @@ def __new__(
# members become proper class variables

for name, value in members.items():
if _is_descriptor(value) or name[0] == "_":
continue

member = value_map.get(value)
if member is None:
member = cls.__new__(cls, name=name, value=value) # type: ignore
Expand Down
6 changes: 4 additions & 2 deletions src/betterproto/plugin/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -72,13 +72,13 @@
)
from betterproto.lib.google.protobuf.compiler import CodeGeneratorRequest

from ..casing import sanitize_name
from ..compile.importing import (
get_type_reference,
parse_source_type_name,
)
from ..compile.naming import (
pythonize_class_name,
pythonize_enum_member_name,
pythonize_field_name,
pythonize_method_name,
)
Expand Down Expand Up @@ -673,7 +673,9 @@ def __post_init__(self) -> None:
# Get entries/allowed values for this Enum
self.entries = [
self.EnumEntry(
name=sanitize_name(entry_proto_value.name),
name=pythonize_enum_member_name(
entry_proto_value.name, self.proto_obj.name
),
value=entry_proto_value.number,
comment=get_comment(
proto_file=self.source_file, path=self.path + [2, entry_number]
Expand Down
8 changes: 8 additions & 0 deletions tests/inputs/enum/enum.proto
Original file line number Diff line number Diff line change
Expand Up @@ -15,3 +15,11 @@ enum Choice {
FOUR = 4;
THREE = 3;
}

// A "C" like enum with the enum name prefixed onto members, these should be stripped
enum ArithmeticOperator {
ARITHMETIC_OPERATOR_NONE = 0;
ARITHMETIC_OPERATOR_PLUS = 1;
ARITHMETIC_OPERATOR_MINUS = 2;
ARITHMETIC_OPERATOR_0_PREFIXED = 3;
}
10 changes: 10 additions & 0 deletions tests/inputs/enum/test_enum.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
from tests.output_betterproto.enum import (
ArithmeticOperator,
Choice,
Test,
)
Expand Down Expand Up @@ -102,3 +103,12 @@ def test_enum_mapped_on_parse():

# bonus: defaults after empty init are also mapped
assert Test().choice.name == Choice.ZERO.name 4D02


def test_renamed_enum_members():
assert set(ArithmeticOperator.__members__) == {
"NONE",
"PLUS",
"MINUS",
"_0_PREFIXED",
}
0