Skip to content

Improved Enum Naming Options to Avoid Collision #191

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

Closed
bowenwr opened this issue Sep 18, 2020 · 3 comments
Closed

Improved Enum Naming Options to Avoid Collision #191

bowenwr opened this issue Sep 18, 2020 · 3 comments
Labels
✨ enhancement New feature or improvement

Comments

@bowenwr
Copy link
Contributor

bowenwr commented Sep 18, 2020

Is your feature request related to a problem? Please describe.

When two components have an enum field with the same name, the second Enum class generated is just incremented with a number. For instance, given a field status, the two generated classes will be Status and Status1. It makes for a rather unhelpful name.

Given spec snippet:

    CheckoutRecord:
      type: object
      properties:
        status:
          type: string
          enum: ['AVAILABLE', 'RESERVED', 'CHECKED_OUT']

    Task:
      type: object
      properties:
        status:
          type: string
          enum: ['RUNNING', 'SUCCEEDED', 'FAILED']

Generates Enum classes:

class Status(str, Enum):
    AVAILABLE = "AVAILABLE"
    RESERVED = "RESERVED"
    CHECKED_OUT = "CHECKED_OUT"
class Status1(str, Enum):
    RUNNING = "RUNNING"
    SUCCEEDED = "SUCCEEDED"
    FAILED = "FAILED"

Describe the solution you'd like

I think the best thing to have for compatibility here might be to add an option to the generator which will prepend the component name to the generated Enum class.

For example, given the example above the two generated Enums would be CheckoutRecordStatus and TaskStatus. Naming collision is very unlikely in this case and the names are much more helpful.

@bowenwr bowenwr added the ✨ enhancement New feature or improvement label Sep 18, 2020
@bowenwr
Copy link
Contributor Author

bowenwr commented Oct 2, 2020

As an addendum, this is more than just a cosmetic nicety. As we add fields to the spec that would generate the same Enum names, order is not guaranteed. Something that was previously Status could become Status1 and break compatibility with code already relying on Status which is now a different Enum set.

@dbanty
Copy link
Collaborator

dbanty commented Oct 2, 2020

Great suggestion, I think we should just always prepend the source of inline (properties) enums to be more obvious. If declared as a top level component it will still use the title / name.

I think semantically, if not always in practice, an inline enum suggests it's only for that one component where a top level component enum suggests it's shared in multiple places (ref).

It will be a breaking change for existing clients, but I think the improvement in usability is worth making it the new standard behavior instead of an opt-in.

@dbanty
Copy link
Collaborator

dbanty commented Nov 10, 2020

Closed with #236, will be available in 0.7.0

@dbanty dbanty closed this as completed Nov 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
✨ enhancement New feature or improvement
Projects
None yet
Development

No branches or pull requests

2 participants