Skip to content

[Kotlin] add apiSuffix configuration#2690

Merged
wing328 merged 12 commits into
OpenAPITools:masterfrom
fj-roman:master
May 7, 2019
Merged

[Kotlin] add apiSuffix configuration#2690
wing328 merged 12 commits into
OpenAPITools:masterfrom
fj-roman:master

Conversation

@fj-roman
Copy link
Copy Markdown
Contributor

@fj-roman fj-roman commented Apr 18, 2019

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, ./bin/openapi3/{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 found in .\bin\windows\.
  • Filed the PR against the correct branch: master, 3.4.x, 4.0.x. Default: master.
  • Copied the technical committee to review the pull request if your PR is targeting a particular programming language.

@bbdouglas @sreeshas @jfiala @lukoyanov @cbornet @jeff9finger @karismann @Zomzog

Description of the PR

Add an option to configure a suffix of generated API classes. Default suffix is still API.

@fj-roman fj-roman changed the title add apiSuffix configuration [Java] add apiSuffix configuration Apr 29, 2019
@fj-roman
Copy link
Copy Markdown
Contributor Author

Can someone please give me a clue about the issue with ci/circleci ?

@Zomzog
Copy link
Copy Markdown
Contributor

Zomzog commented Apr 29, 2019

For the last build, it looks like a CircleCi cache issue may be a core member can trigger a build without cache.

But for the first error, you must regenerate all impacted sample (R, c#, go...)

@karismann
Copy link
Copy Markdown
Contributor

it seem like there's a regression with your change when apisuffix is not set : for example FakeApi become Fake. Thats'why you have a lot of error with sample regenerate checking shell in Cirecle/CI.
Please try to rerun one of the sample locally (like java-petstore-jersey2.sh) to try to debug it

protected Set<String> languageSpecificPrimitives = new HashSet<String>();
protected Map<String, String> importMapping = new HashMap<String, String>();
protected String modelPackage = "", apiPackage = "", fileSuffix;
protected String modelPackage = "", apiPackage = "", apiSuffix = "", fileSuffix;
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.

you forgot to define the default value of apiSuffix :
apiSuffix = "Api"
that should resolve your CIRCLE/CI errors

return "DefaultApi";
}
return camelize(name) + "Api";
return (this.apiSuffix.isEmpty() ? camelize(name) : camelize(name) + this.apiSuffix);
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.

that would be great to remove the toApiName overriding method in AbstractJavaCodegen class to be able to have this feature in most of java generators

Copy link
Copy Markdown
Contributor

@karismann karismann left a comment

Choose a reason for hiding this comment

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

LGTM

@fj-roman
Copy link
Copy Markdown
Contributor Author

Thanks for the help and code review!

@wing328
Copy link
Copy Markdown
Member

wing328 commented Apr 30, 2019

@fj-roman thanks for adding the functionality 👍

@karismann thanks for the review.

From what I understand, apiSuffix is added as a global option but only the Java generators support this option so far. Instead of making it a global option from day one, what about adding the option only to the Java generators (Abstract Java class) to start with?

In other words, apiSuffix should be introduced as a generator-specified option.

@fj-roman
Copy link
Copy Markdown
Contributor Author

@wing328 fair point. My main focus is to provide this feature for the KotlinSpringServerCodegen. I will refactor the the PR to move this feature to the AbstractKotlinCodegen. Maybe I can provide it to all Kotlin generators within one PR.

Should I close this PR and open later a new one, or should I just edit the description after I complete the refactoring?

@fj-roman fj-roman closed this May 6, 2019
@wing328
Copy link
Copy Markdown
Member

wing328 commented May 6, 2019

@fj-roman why closed the PR?

@fj-roman fj-roman reopened this May 6, 2019
@wing328 wing328 changed the title [Java] add apiSuffix configuration [Kotlin] add apiSuffix configuration May 7, 2019
@wing328 wing328 added this to the 4.0.0 milestone May 7, 2019
@wing328
Copy link
Copy Markdown
Member

wing328 commented May 7, 2019

Tested locally and it works as expected 👍

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.

5 participants