openapi-generator icon indicating copy to clipboard operation
openapi-generator copied to clipboard

[SPRING] Generate inner enum for query and path params

Open Zomzog opened this issue 3 years ago • 7 comments

I've added the inner class. The static * import for the delegate was the only solution I found for importing them.

PR checklist

  • [x] Read the contribution guidelines.
  • [x] Pull Request title clearly describes the work in the pull request and Pull Request description provides details about how to validate the work. Missing information here may result in delayed response from the community.
  • [x] Run the following to build the project and update samples:
    ./mvnw clean package 
    ./bin/generate-samples.sh
    ./bin/utils/export_docs_generators.sh
    
    Commit all changed files. This is important, as CI jobs will verify all generator outputs of your HEAD commit as it would merge with master. These must match the expectations made by your contribution. You may regenerate an individual generator by passing the relevant config(s) as an argument to the script, for example ./bin/generate-samples.sh bin/configs/java*. For Windows users, please run the script in Git BASH.
  • [x] File the PR against the correct branch: master (6.1.0) (minor release - breaking changes with fallbacks), 7.0.x (breaking changes without fallbacks)
  • [x] If your PR is targeting a particular programming language, @mention the technical committee members, so they are more likely to review the pull request.

Spring committee : @cachescrubber (2022/02) @welshm (2022/02) @MelleD (2022/02) @atextor (2022/02) @manedev79 (2022/02) @javisst (2022/02) @borsch (2022/02) @banlevente (2022/02)

Zomzog avatar Sep 04 '22 22:09 Zomzog

I think you need your other change in first to get these tests to pass - but the change looks good otherwise

welshm avatar Sep 12 '22 20:09 welshm

@welshm and @Zomzog Short question. Does it really make sense to generate the enum in the API class/interface? Wouldn't it be better to have a separate file for the DTOs?

MelleD avatar Sep 13 '22 09:09 MelleD

@welshm and @Zomzog Short question. Does it really make sense to generate the enum in the API class/interface? Wouldn't it be better to have a separate file for the DTOs?

That's a good callout - a separate file where the DTOs are located is probably a better result. It will make generating the imports a little harder

welshm avatar Sep 13 '22 12:09 welshm

@welshm and @Zomzog Short question. Does it really make sense to generate the enum in the API class/interface? Wouldn't it be better to have a separate file for the DTOs?

That's a good callout - a separate file where the DTOs are located is probably a better result. It will make generating the imports a little harder

I would leave it as it is. These enums are operations/class specific. Assume situation when you have two different operations from two different controllers, they both receive parameter action with enum type, but those have different sets of allowed values

IMHO: there should be a rule - as long as client declare enum as global type(components -> schemas -> MyEnum) it should be in separate class, in other cases(enum for model's field or enum for operation's parameter only) it should be inner enum to class, where it should be used

borsch avatar Sep 16 '22 11:09 borsch

@welshm and @Zomzog Short question. Does it really make sense to generate the enum in the API class/interface? Wouldn't it be better to have a separate file for the DTOs?

That's a good callout - a separate file where the DTOs are located is probably a better result. It will make generating the imports a little harder

I would leave it as it is. These enums are operations/class specific. Assume situation when you have two different operations from two different controllers, they both receive parameter action with enum type, but those have different sets of allowed values

IMHO: there should be a rule - as long as client declare enum as global type(components -> schemas -> MyEnum) it should be in separate class, in other cases(enum for model's field or enum for operation's parameter only) it should be inner enum to class, where it should be used

The issue is more on the client side like feign. Then you have to use an "interface enum" looks a little bit confusing.

MelleD avatar Sep 16 '22 11:09 MelleD

@welshm and @Zomzog Short question. Does it really make sense to generate the enum in the API class/interface? Wouldn't it be better to have a separate file for the DTOs?

That's a good callout - a separate file where the DTOs are located is probably a better result. It will make generating the imports a little harder

I would leave it as it is. These enums are operations/class specific. Assume situation when you have two different operations from two different controllers, they both receive parameter action with enum type, but those have different sets of allowed values IMHO: there should be a rule - as long as client declare enum as global type(components -> schemas -> MyEnum) it should be in separate class, in other cases(enum for model's field or enum for operation's parameter only) it should be inner enum to class, where it should be used

The issue is more on the client side like feign. Then you have to use an "interface enum" looks a little bit confusing.

nope, that's a common issue. you can't declare multiple enums with same name within same package

borsch avatar Sep 17 '22 05:09 borsch

It was harder than expected because some enums are weird. I've done some changes to the case of an ArrayEnum. It looks like it has mostly an impact on documentation for other generators. I don't get why node3 failed. I will try to look at it later.

Zomzog avatar Sep 25 '22 23:09 Zomzog