Skip to content

Added apiNameSuffix parameter for JaxRsSpec code generator #7160

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

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

Conversation

ndurette
Copy link

@ndurette ndurette commented Dec 12, 2017

PR checklist

  • Read the contribution guidelines.
  • Ran the shell script under ./bin/ to update Petstore sample so that CIs can verify the change. (For instance, only need to run ./bin/{LANG}-petstore.sh and ./bin/security/{LANG}-petstore.sh if updating the {LANG} (e.g. php, ruby, python, etc) code generator or {LANG} client's mustache templates). Windows batch files can be fold .und in .\bin\windows\.
  • Filed the PR against the correct branch: 3.0.0 branch for changes related to OpenAPI spec 3.0. Default: master.
  • Copied the technical committee to review the pull request if your PR is targeting a particular programming language.

Description of the PR

Issue: #7150

I added an additional property to specify an api name suffix instead of always using "Api".
I only changed the JaxRsSpec generator but it will also affect generator that are not reimplementing "toApiName()".

I did run the script as mentioned but I didn't include to change as it changed unrelated stuff.
You might want to run all the script on master.

@bbdouglas @JFCote @sreeshas @jfiala @lukoyanov @cbornet

@JFCote
Copy link
Contributor

JFCote commented Dec 13, 2017

@ndurette I think it would be nice to see the changes on at least one generator (run the shell script). Also, I would be nice to add a .sh file that use your new parameter so we could the the effect it have compared to the default.

@ndurette
Copy link
Author

@JFCote I added a new script to generate a new server using my feature. I also ran the previous "jaxrs-spec-petstore-server" scripts.

Copy link
Contributor

@JFCote JFCote left a comment

Choose a reason for hiding this comment

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

You code seems ok! But like I said in the review, I'm not sure if it's ok to create new "generator wide" parameters since it can impact all generators. I'm also not sure if all generator (client for example) should have the concept of api suffix.

I'll let @wing328 and @cbornet evaluate the situation too! Thanks

@@ -74,6 +74,10 @@
description = CodegenConstants.API_PACKAGE_DESC)
private String apiPackage;

@Option(name = {"--api-name-suffix"}, title = "api name suffix",
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this should be here at all. If I'm not mistaken, these are the general swagger-codegen wide parameters that all generator should support. I think you only need to add you parameters to your specific codegen. See this for an example:

https://github.com/swagger-api/swagger-codegen/blob/master/modules/swagger-codegen/src/main/java/io/swagger/codegen/languages/JavaPlayFrameworkCodegen.java#L22

Copy link
Contributor

Choose a reason for hiding this comment

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

In fact, I check all your code, it seems you are indeed trying to create an "all generators" parameter... I'm not sure it is really safe to add that kind of parameters. I will add @wing328 and @cbornet to the conversation to have their advices.

@ndurette
Copy link
Author

In my case adding the property in JavaJAXRSSpecServerCodegen would do it.
Do you think it can be valuable to have this additional property in AbstractJavaJAXRSServerCodegen?

The idea behind this modification was to copy the "model-name-suffix" feature. But I agree that it might not fit for every generator.

I'll wait for comments and then make the changes as needed.

@wing328
Copy link
Contributor

wing328 commented Jan 15, 2018

The idea behind this modification was to copy the "model-name-suffix" feature. But I agree that it might not fit for every generator.

Given that it's only targeting a specified generator, please add the option to JavaJAXRSSpecServerCodegen for the time being. If there is more demand to add this feature to other generators, we'll add it one by one.

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

Successfully merging this pull request may close these issues.

4 participants