Skip to content
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

[CLI] Improvements for meta and list command #799

Merged
merged 16 commits into from
Aug 29, 2018

Conversation

jmini
Copy link
Member

@jmini jmini commented Aug 13, 2018

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

Description of the PR

To reproduce:

Create a my-codegen-openapi-generator artefact using the meta command as described here:
https://github.com/OpenAPITools/openapi-generator/blob/master/docs/customization.md#creating-a-new-template

Change 1/: If you create a generator that have no type (CLIENT, SERVER, DOCUMENTATION, CONFIG, OTHER) it should be displayed when you run the command:

(classpath in the windows format, for linux or Mac replace the ; with a :)

java -cp out/generators/my-codegen/target/my-codegen-openapi-generator-1.0.0.jar;modules/openapi-generator-cli/target/openapi-generator-cli.jar org.openapitools.codegen.OpenAPIGenerator

The change introduces the category UNDIFINED for this case.
It also skip a category if not generator is present for it.

Change 2/: the meta command is now from type OTHER:

Example MyCodegenGenerator:

  public CodegenType getTag() {
    return CodegenType.OTHER;
  }

Change 3/: meta script is added to ensure-up-to-date

@jimschubert
Copy link
Member

@jmini what is the problem you're seeing that you're trying to resolve with this?

If you're adding the UNDEFINED category for custom generators that return null for getType(), I have to ask why. For a generator project, I don't think we should ever want to consider an "undefined" generator type like this (especially when we already have an "OTHER" category.

Also, I don't think the meta generator should default to OTHER. What's the reason for that change? Are you trying to force users to define their generator types after invoking meta? Why not just add a command to the task and allow users to define the CodegenType value at generation time?

A side note… while looking at the customization doc, I found that the last command before Selective generation is incorrect. Would you mind changing it as part of your branch? It should be:

java -cp out/generators/my-codegen/target/my-codegen-openapi-generator-1.0.0.jar:modules/openapi-generator-cli/target/openapi-generator-cli.jar \ 
  org.openapitools.codegen.OpenAPIGenerator generate -g my-codegen   \
  -i https://raw.githubusercontent.com/openapitools/openapi-generator/master/modules/openapi-generator/src/test/resources/2_0/petstore.yaml \
  -o ./out/myClient

(notice the output path and org.openapitools instead of io.openapitools.

@jimschubert
Copy link
Member

Also, directions in the README for this generator are incorrect. Would you mind changing them on your branch as well?

java -cp /path/to/openapi-generator-cli.jar:/path/to/your.jar org.openapitools.codegen.Codegen -g {{name}} -i /path/to/openapi.yaml -o ./test

Should match what I posted above:

java -cp /path/to/openapi-generator-cli.jar:/path/to/your.jar \
    org.openapitools.codegen.OpenAPIGenerator generate -g {{name}} \
    -i /path/to/openapi.yaml -o ./test 

Notice: entry class is incorrect, and example is missing generate command.

@jmini
Copy link
Member Author

jmini commented Aug 14, 2018

A side note… while looking at the customization doc, I found that the last command before Selective generation is incorrect. (..) notice the output path and org.openapitools instead of io.openapitools.

I know, see my conversation on gitter yesterday with @pgnaleen. => I have planned doc fixing in an other PR.

For a generator project, I don't think we should ever want to consider an "undefined" generator type like this (especially when we already have an "OTHER" category).

I agree, but this is not something you can prevent... getTag() can return null. Not displaying the generator at all is wrong.

If we think about #137, I guess that this getTag() (that we should rename getGeneratorType() or something like this) is the switch you are looking for.

Maybe we should treat null as OTHER, but I am not sure where...
And if someone is using null we should log a WARNING.

Also, I don't think the meta generator should default to OTHER. What's the reason for that change? Are you trying to force users to define their generator types after invoking meta? Why not just add a command to the task and allow users to define the CodegenType value at generation time?

My idea behind the change was:

  • We do not know what they are trying to create (a client, a server)
  • The user should change it.
  • In the OTHER category they will better see their generator.

I like the idea with the option. I need to figure out how we can add this.

Also, directions in the README for this generator are incorrect. Would you mind changing them on your branch as well?

Nice catch with the README template. Yes I can update it.

@jimschubert
Copy link
Member

@jmini I see your point about the null types, although I really think this is an edge case.

Could we name it "UNSPECIFIED" or "UNTAGGED" instead. My concern is mainly that "UNDEFINED" looks like the CLI or the generator module are exposing a programming error.

@jmini
Copy link
Member Author

jmini commented Aug 15, 2018

See #817 for the docs.

@@ -116,7 +116,7 @@
</dependencies>
<properties>
<project.build.sourceEncoding>UTF-8</project.build.sourceEncoding>
<openapi-generator-version>3.0.0-SNAPSHOT</openapi-generator-version>
<openapi-generator-version>3.2.2-SNAPSHOT</openapi-generator-version>
Copy link
Member

Choose a reason for hiding this comment

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

I'll take note of this during the release process.

Copy link
Member Author

Choose a reason for hiding this comment

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

lib/ is generated (output of the meta command of the CLI). It is updated with ensure-up-to-date.

@jmini jmini added the WIP Work in Progress label Aug 21, 2018
@jmini
Copy link
Member Author

jmini commented Aug 21, 2018

WIP: not all the changes requested here, are implemented.

@wing328 wing328 modified the milestones: 3.2.2, 3.2.3 Aug 22, 2018
@jmini jmini removed the WIP Work in Progress label Aug 23, 2018
@jmini
Copy link
Member Author

jmini commented Aug 23, 2018

Everything is implemented, this PR is ready for review.

@jmini
Copy link
Member Author

jmini commented Aug 28, 2018

I will merge this later today if nobody else wants to comment.

@jmini jmini merged commit 8e1e05e into OpenAPITools:master Aug 29, 2018
@jmini jmini changed the title Fixes for meta and list command [CLI] Improvements for meta and list command Aug 29, 2018
A-Joshi pushed a commit to ihsmarkitoss/openapi-generator that referenced this pull request Feb 27, 2019
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.

None yet

3 participants