Skip to content

Support x-enum-varnames extension - #692

Closed
reedom wants to merge 4 commits into
oapi-codegen:mainfrom
reedom:feature/enum-varnames
Closed

Support x-enum-varnames extension#692
reedom wants to merge 4 commits into
oapi-codegen:mainfrom
reedom:feature/enum-varnames

Conversation

@reedom

@reedom reedom commented Jul 26, 2022

Copy link
Copy Markdown
Contributor

Summary:

Support x-enum-varnames extension, derived from openapi-generator.

With it, we can supply other enum names for the corresponding values.

components:
  schemas:
    Object:
      properties:
        category:
          type: integer
          enum: [0, 1, 2]
          x-enum-varnames:
            - notice
            - warning
            - urgent

After code generation you will get this result:

// Defines values for ObjectCategory.
const (
	Notice  ObjectCategory = 0
	Urgent  ObjectCategory = 2
	Warning ObjectCategory = 1
)

// ObjectCategory defines model for Object.Category.
type ObjectCategory int

reedom added 4 commits July 26, 2022 16:17
@amery

amery commented Oct 28, 2022

Copy link
Copy Markdown
Contributor

@reedom I took the liberty to rebase your branch on top of master. should I make a new pull request @deepmap-marcinr?

master...amery:oapi-codegen:feature/enum-varnames

@reedom

reedom commented Oct 28, 2022

Copy link
Copy Markdown
Contributor Author

@amery
Thanks, that sounds nice.

@amery

amery commented Oct 28, 2022

Copy link
Copy Markdown
Contributor

@amery Thanks, that sounds nice.

@reedom, I did notice it uses the value as varname if x-enum-varnames is not defined. that feels like an undesired side effect

@reedom

reedom commented Oct 28, 2022

Copy link
Copy Markdown
Contributor Author

@amery

I did notice it uses the value as varname if x-enum-varnames is not defined. that feels like an undesired side effect

Excuse me, I don't get the point.
Could you explain in more detail, possibly with a code snippet?

@amery

amery commented Oct 28, 2022

Copy link
Copy Markdown
Contributor

@reedom for me it doesn't render the consts unless I add a path. so:

paths:
  '/types':
    get:
      responses:
        '200':
          description: OK
          content:
            'application/json':
              schema:
                type: array
                itemsUnique: true
                items:
                  $ref: '#/components/schemas/ObjectType'

components:
  schemas:
    ObjectType:
      title: ObjectType
      type: string
      enum:
        - 'A'
        - 'B'
        - 'C'

used to produce:

const (
        ObjectTypeA ObjectType = "A"
        ObjectTypeB ObjectType = "B"
        ObjectTypeC ObjectType = "C"
)

now it produces

const (
        A ObjectType = "A"
        B ObjectType = "B"
        C ObjectType = "C"
)

unless there is something else at play here

@reedom

reedom commented Oct 28, 2022

Copy link
Copy Markdown
Contributor Author

@amery

It seems doesn't relate to this patch and I think that's how always-prefix-enum-values: true works.

package: sample
generate:
  models: true
compatibility:
  always-prefix-enum-values: true

@amery

amery commented Oct 28, 2022

Copy link
Copy Markdown
Contributor

perfect, thank you @reedom. I had the feeling I had missed something

@amery

amery commented Oct 29, 2022

Copy link
Copy Markdown
Contributor

@deepmap-marcinr @reedom master...amery:oapi-codegen:feature/enum-varnames has been rebased over 1.12.2

@stevenh stevenh left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks like some debugging got left in.

Comment thread pkg/codegen/schema.go
Comment on lines +404 to +406
if 0 < len(schema.ExtensionProps.Extensions) {
//fmt.Fprintf(os.Stderr, "%#v\n\n", schema)
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Remove old debugging?

Suggested change
if 0 < len(schema.ExtensionProps.Extensions) {
//fmt.Fprintf(os.Stderr, "%#v\n\n", schema)
}
if 0 < len(schema.ExtensionProps.Extensions) {
//fmt.Fprintf(os.Stderr, "%#v\n\n", schema)
}

@Antonboom

Copy link
Copy Markdown
Contributor

+1 to x-enum-varnames feature

@amery

amery commented Nov 15, 2022

Copy link
Copy Markdown
Contributor

Looks like some debugging got left in.

I just rebased and amended my branch to exclude this. thanks @stevenh

master...amery:oapi-codegen:feature/enum-varnames

@ghost

ghost commented Nov 15, 2022

Copy link
Copy Markdown

Can you resolve those conflicts above, then I'll happily merge this

@amery

amery commented Nov 15, 2022

Copy link
Copy Markdown
Contributor

Can you resolve those conflicts above, then I'll happily merge this

@deepmap-marcinr should I make a new pull request from my branch?

@amery amery mentioned this pull request Nov 30, 2022
@amery

amery commented Nov 30, 2022

Copy link
Copy Markdown
Contributor

Can you resolve those conflicts above, then I'll happily merge this

@deepmap-marcinr should I make a new pull request from my branch?

I just made one, #880.

@amery

amery commented Dec 17, 2022

Copy link
Copy Markdown
Contributor

@deepmap-marcinr as #880 was merged, this one can be closed

@ghost

ghost commented Jan 18, 2023

Copy link
Copy Markdown

This looks useful.

I've not merged this yet because I have a few stability concerns.

Your enum naming is dependent on order, and it's really easy to mess up by reordering one list and not the other. I think a mapping would work better, in your example:

components:
  schemas:
    Object:
      properties:
        category:
          type: integer
          enum: [0, 1, 2]
          x-enum-varnames:
            "1":  notice
            "2": warning
            "3": urgent

The complications in this approach are that key names must be expressible in json, so they must be strings, even though YAML supports other types. However, I'm not going to push back on this, because the tradeoffs are complex.

Comment thread pkg/codegen/schema.go
for _, key := range []string{extEnumVarNames, extEnumNames} {
if _, ok := schema.ExtensionProps.Extensions[key]; ok {
if extEnumNames, err := extParseEnumVarNames(schema.ExtensionProps.Extensions[key]); err == nil {
enumNames = extEnumNames

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Right now, your extension completely replaces the enum names that are autogenerated. What happens when the provided names array has a different length than possible enum values? I think we'll end up creating invalid enums. Can you, at a minimum, error out on cardinality mismatch?

@mromaszewicz

Copy link
Copy Markdown
Member

Thank you for contributing, and I'm very sorry for taking so long to get to this PR. At this point, the code has changed so much that it's no longer relevant because this was superseded by PR #880 which was merged in Dec 2022. The x-enum-varnames extension is fully supported, with a working example at examples/extensions/xenumnames/.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants