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

Fix enum name processing and add uppercase flag #257

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

slessans
Copy link
Contributor

@slessans slessans commented Dec 21, 2023

This PR fixes the following bugs in the processing of enum names:

  1. the snake_case conversion function did not properly handle names with all uppercase eg DESC_NULLS_LAST was being converted into LAST
  2. The enum generator was not calling process_name and so could not be hooked into the plugin customization pipeline
  3. The enum generator did not respect the convert_to_snake_case option

This PR additionally adds the convert_enums_names_to_upper_snake_case option to convert enum members to upper snake case like:

class SomeEnum(str, Enum):
    FOO_BAR = "fooBar"
    BAZ_BOW = "bazBow"
    TO_STATUS = "toStatus"

I think this should be a core option rather than a plugin since it is more idiomatic. It also avoid enum name conflicts. For instance I had an enum with title and name as members, which conflicted with the built-in enum methods of the same name. I set the flag to default False for backwards compat.

Copy link
Contributor

@mat-sop mat-sop left a comment

Choose a reason for hiding this comment

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

Hey, it looks like a lot of different topics for 1 pr:

  1. the snake_case conversion function did not properly handle names with all uppercase eg DESC_NULLS_LAST was being converted into LAST

This looks quite serious, but I'm not sure if we should mark this as a breaking change, or just a fix.

  1. The enum generator was not calling process_name and so could not be hooked into the plugin customization pipeline

You can use generate_enum hook, but I agree that using process_name here can be a nice improvement.

  1. The enum generator did not respect the convert_to_snake_case option

This PR additionally adds the convert_enums_names_to_upper_snake_case option to convert enum members to upper snake case like:

The fact that convert_to_snake_case doesn't apply to enums definitely should be better documented. I'm not sure if we should implement it, same for convert_enums_names_to_upper_snake_case. In your implementation you can't have current behavior - snake case for arguments and result/input fields, not changed enums. Maybe it should be a plugin?

For instance I had an enum with title and name as members, which conflicted with the built-in enum methods of the same name.

We should escape those by adding _, similar to how we already handle overlap with pydantic model's attributes and method.

@rafalp what do you think?

@slessans
Copy link
Contributor Author

@mat-sop @rafalp Let me know how you want me to break it up/structure it and I am happy to do it, I just needed to unblock a project.

@rafalp
Copy link
Contributor

rafalp commented Jan 19, 2024

Hello, apologies for delay in tacking this:

  1. Valid bug
  2. Enum names should go through process_name like other names.

Name conflict with Python keywords should be solved by appending the _ to final name, eg title_

This PR additionally adds the convert_enums_names_to_upper_snake_case option to convert enum members to upper snake case like

This is odd. Are you naming your GraphQL enum members like title or name instead of TITLE and NAME? If thats the case, its schema design issue and not something codegen should handle out of the box.

@rafalp rafalp added the waiting Waiting for action from OP label Jan 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
waiting Waiting for action from OP
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants