Expanding customization docs by philsturgeon · Pull Request #283 · OpenAPITools/openapi-generator

philsturgeon · 2018-06-11T12:56:01Z

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: Default: master.
Copied the technical committee to review the pull request if your PR is targeting a particular programming language.

Description of the PR

As a new user I was very confused on how to either modify, override or create a new library. The existing docs were brief and had requirred knowledge that new users will not have, and it took a lot of trial and error to make progress.

Also the wording seemed off. It kept talking about "client" when this project creates more than just clients, used "language" when some templates are not a language, and used library to mean a few seeminly different things. I've tried to consolidate the wording around "template" and "codegen".

The codegen is the Java code, and then you give it a "name" which is used for the template. That's the template folder it will live in, so it seems to work.

Feel free to use this as a base and edit it out, but this will all make a lot more sense to beginners. :)

jmini · 2018-06-11T13:13:32Z

Thank you a lot for this contribution. Improving documentation is really valuable.
It looks good to me and I am sure it will help other users!

wing328 · 2018-06-11T13:50:20Z

minor typo: pogramming => programming

I was talking about pogs!

🤣 fixed it! Thank you.

As a new user I was very confused on how to either modify, override or create a new library. The existing docs were brief and had requirred knowledge that new users will not have, and it took a lot of trial and error to make progress. Also the wording seemed off. It kept talking about "client" when this project creates more than just clients, used "language" when some templates are not a language, and used library to mean a few seeminly different things. I've tried to consolidate the wording around "template" and "codegen". The codegen is the Java code, and then you give it a "name" which is used for the template. That's the template folder it will live in, so it seems to work. Feel free to use this as a base and edit it out, but this will all make a lot more sense to beginners. :)

jimschubert

Thanks for updating this doc! This is a much needed change. I've added some comments, mostly about text that was there before and a couple suggestions for further clarifications.

jimschubert · 2018-06-12T01:34:12Z

 ```sh
 java -jar modules/openapi-generator-cli/target/openapi-generator-cli.jar meta \
-  -o output/myLibrary -n myClientCodegen -p com.my.company.codegen
+  -o codegens/customCodegen -n myTemplate -p com.my.company.codegen


I don't know that codegens/customCodegen is any better than output/myLibrary, and I worry that it might lead to confusion. codegens as a directory may make it seem like this is some conventional or required directory name, while output is commonly understood as just a placeholder for your output. This could probably be resolved if you add that the output directory can be anything?

Also, -n myTemplate is not the convention we normally use, and yet myClientCodegen is rather outdated (I think since swagger-codegen 2.0.0 or so). MyTemplate as a class name doesn't make sense here because you're generating the codegen type and not a template type.

Compare your output:

$ tree codegens/ codegens/ └── customCodegen ├── README.md ├── pom.xml └── src └── main ├── java │   └── com │   └── my │   └── company │   └── codegen │   └── MytemplateGenerator.java └── resources ├── META-INF │   └── services │   └── org.openapitools.codegen.CodegenConfig └── myTemplate ├── api.mustache ├── model.mustache └── myFile.mustache 12 directories, 7 files

With the previous:

$ tree output/ output/ └── myLibrary ├── README.md ├── pom.xml └── src └── main ├── java │   └── com │   └── my │   └── company │   └── codegen │   └── MyclientcodegenGenerator.java └── resources ├── META-INF │   └── services │   └── org.openapitools.codegen.CodegenConfig └── myClientCodegen ├── api.mustache ├── model.mustache └── myFile.mustache 12 directories, 7 files

If -n is provided something equivalent to what you would pass as --generator-name (previously --lang), you would get a reasonable output:

$ tree output/ output/ └── finatra-server ├── README.md ├── pom.xml └── src └── main ├── java │   └── us │   └── jimschubert │   └── generators │   └── FinatraGenerator.java └── resources ├── META-INF │   └── services │   └── org.openapitools.codegen.CodegenConfig └── finatra ├── api.mustache ├── model.mustache └── myFile.mustache 11 directories, 7 files

Compare this to the files generated by the new.sh script I have under the project root, which is intended for bootstrapping new generators within our project:

./new.sh -n finatra -s -t

You'll notice that the output from new.sh and the output from the last command above are relatively similar. That is, it would be much easier for someone to contribute their custom generated code back to the project following those conventions (namely -n matching generator-name/lang formats).

jimschubert · 2018-06-12T01:35:04Z

 ```

-This will write, in the folder `output/myLibrary`, all the files you need to get started, including a `README.md. Once modified and compiled, you can load your library with the codegen and generate clients with your own, custom-rolled logic.
+This will create a new directory `codegens/customCodegen`, with all the files you need to get started - including a `README.md. Once modified and compiled, you can use your new codegen just like any other, but with your own custom-rolled logic.


I know this existed previously, but could you close the backtick around README.md or remove the unclosed one and leave regular text?

jimschubert · 2018-06-12T01:49:53Z

+This will create a new directory `codegens/customCodegen`, with all the files you need to get started - including a `README.md. Once modified and compiled, you can use your new codegen just like any other, but with your own custom-rolled logic.

-You would then compile your library in the `output/myLibrary` folder with `mvn package` and execute the codegen like such:
+To compile your library, enter the `codegens/customCodegen` directory, run `mvn package` and execute the generator like such:


I know it's not a change of yours, but "like such" should probably be changed as well. I think it would be like, like so, or just and execute the generator: without any decoration. like such is vernacular/slang that I don't think most people have heard.

jimschubert · 2018-06-12T01:51:16Z


 ```sh
-# OpenAPI Generator Ignore
+# .openapi-generator-ignore


This isn't a necessary change, as this file is not required to be named .openapi-generator-ignore.

jimschubert · 2018-06-12T01:53:53Z

+### Modifying a template

-You can look at `modules/openapi-generator/src/main/resources/${your-language}` for examples. To make your own templates, create your own files and use the `-t` flag to specify your template folder.  It actually is that easy.
+Clone OpenAPI Generator and take a look at the following directory: `modules/openapi-generator/src/main/resources/${template}`. In here you'll see all of the generators available, for most programming languages, web application frameworks and web servers.


The ${template} should be changed back to ${your-language} or something similar. The point of the previous text is that, for instance, if you're trying to modify csharp, you would look at modules/openapi-generator/src/main/resources/csharp. Changing this to ${template} seems more confusing to me, and adding the following line that all generators exist under this directory would be incorrect.

I thought this was clear enough, but I've expanded it:

Clone OpenAPI Generator and take a look at the following directory: modules/openapi-generator/src/main/resources/${template}. In here you'll see all of the generators available, for most programming languages, web application frameworks and web servers. For example, if you are looking for the C# template, it's named csharp.

It's not just language, so saying your-language doesnt make any sense.

jimschubert · 2018-06-12T01:55:26Z

+Clone OpenAPI Generator and take a look at the following directory: `modules/openapi-generator/src/main/resources/${template}`. In here you'll see all of the generators available, for most programming languages, web application frameworks and web servers.

-### Making your own codegen modules
+Templates consist of multiple mustache files. Mustache is a simple, readable templating language, and the engine used is [jmustache](https://github.com/samskivert/jmustache).


simple, readable doesn't really add to this sentence. It makes it sound like we're trying to convince the reader of our choice to use Mustache as a templating language.

I'm trying to let folks know they shouldnt be scared of getting in there to play. 🤷‍♂️

jimschubert · 2018-06-12T01:57:37Z

+
+```sh
+mkdir templates
+cp -r modules/openapi-generator/src/main/resources/${template} templates/


I don't know if it's helpful to add to this doc or what… but if you install refined github in Google Chrome, you get a nifty Download button under every folder as you browse GitHub. I use this often, as you can easily browse to your desired generator that you'd like to extend under modules/openapi-generator/src/main/resources/ and download the folder directly.

I don't wanna push people towards a browser extension when a single command would also explain things just fine.

jimschubert · 2018-06-12T01:59:39Z

+  -t ./templates/ -g ruby -i ./foo.yml -o ./out/ruby
+```
+
+_**Note:** You cannot use this approach to create new templates, only override existing ones._


It might be helpful to add something about running new.sh in the project root for contributing new templates.

Could you add that note after this PR? Not sure what you'd like to say.

philsturgeon · 2018-06-12T16:10:14Z

Implemented feedback for most of the raised points!

jimschubert · 2018-06-12T18:55:58Z

+### Modifying a template

-You can look at `modules/openapi-generator/src/main/resources/${your-language}` for examples. To make your own templates, create your own files and use the `-t` flag to specify your template folder.  It actually is that easy.
+Clone OpenAPI Generator and take a look at the following directory: `modules/openapi-generator/src/main/resources/${template}`. In here you'll see all of the generators available, for most programming languages, web application frameworks and web servers. For example, if you are looking for the C# template, it's named `csharp`.


This still needs to be updated. You're pointing the user to:

modules/openapi-generator/src/main/resources/csharp (where the reader will intuit ${template} is replaced with csharp), and saying that this includes "all of the generators available".

The "all of the generators available" is up a directory, at modules/openapi-generator/src/main/resources.

So, either ${template} would need to be removed, or the description about the directory needs to be modified to accommodate (That is, something like "in here, you'll find all of the templates specific to the generator you plan to modify").

English speakers will read this and go "Oh, that refers to the resource directory" but we have many non-English speakers who will be confused by these three introductory sentences.

jimschubert · 2018-06-12T18:56:55Z

 Examples:

 ```sh
-# OpenAPI Generator Ignore


This shouldn't be removed, either. It's common for files to include a header explaining what the file is.

philsturgeon · 2018-06-12T19:22:57Z

It’d be great if you could merge this and make whatever little tweaky trivialities need to be done for you to be entirely happy with it. Got a race to get ready for and already spent too much time on this.

…

-- Phil Sturgeon @philsturgeon

On Jun 12, 2018, at 14:57, Jim Schubert ***@***.***> wrote: @jimschubert requested changes on this pull request. In docs/customization.md: > -You can look at `modules/openapi-generator/src/main/resources/${your-language}` for examples. To make your own templates, create your own files and use the `-t` flag to specify your template folder. It actually is that easy. +Clone OpenAPI Generator and take a look at the following directory: `modules/openapi-generator/src/main/resources/${template}`. In here you'll see all of the generators available, for most programming languages, web application frameworks and web servers. For example, if you are looking for the C# template, it's named `csharp`. This still needs to be updated. You're pointing the user to: modules/openapi-generator/src/main/resources/csharp (where the reader will intuit ${template} is replaced with csharp), and saying that this includes "all of the generators available". The "all of the generators available" is up a directory, at modules/openapi-generator/src/main/resources. So, either ${template} would need to be removed, or the description about the directory needs to be modified to accommodate (That is, something like "in here, you'll find all of the templates specific to the generator you plan to modify"). English speakers will read this and go "Oh, that refers to the resource directory" but we have many non-English speakers who will be confused by these three introductory sentences. In docs/customization.md: > @@ -95,7 +112,6 @@ The ignore file allows for better control over overwriting existing files than t Examples: ```sh -# OpenAPI Generator Ignore This shouldn't be removed, either. It's common for files to include a header explaining what the file is. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or mute the thread.

user

jimschubert · 2018-06-12T22:09:07Z

@philsturgeon I'll go ahead and merge this and make tweaks in a separate PR. Generally, I don't approve things that result in confusingly-worded documentation, but I can tell you're frustrated by the feedback. My intentions aren't to annoy you with requests, they're to help improve the documentation for which you've opened a PR to improve.

philsturgeon · 2018-06-13T00:55:38Z

Oh I understand ours trying to help, I just didn’t know how to resolve one and the other was minor. This file was made 99% more clear with the PR so thanks for adding that extra 1%! 😎

…

-- Phil Sturgeon @philsturgeon

On Jun 12, 2018, at 18:09, Jim Schubert ***@***.***> wrote: Merged #283. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

jmini · 2018-06-13T11:38:58Z

@philsturgeon thank you a lot for your time and for this contribution!

philsturgeon mentioned this pull request Jun 11, 2018

Creating templates is confusingly documented #276

Closed

jmini approved these changes Jun 11, 2018

View reviewed changes

wing328 reviewed Jun 11, 2018

View reviewed changes

jimschubert reviewed Jun 12, 2018

View reviewed changes

Added feedback to customization improvements

65c2921

jimschubert previously requested changes Jun 12, 2018

View reviewed changes

jimschubert merged commit 2ff81ca into OpenAPITools:master Jun 12, 2018

jimschubert mentioned this pull request Jun 12, 2018

Update template modification docs #291

Merged

4 tasks

wing328 added this to the 3.0.2 milestone Jun 13, 2018

philsturgeon deleted the customization-docs branch August 2, 2018 18:37

Uh oh!

Uh oh!

Conversation

philsturgeon commented Jun 11, 2018

PR checklist

Description of the PR

Uh oh!

jmini commented Jun 11, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jimschubert left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

philsturgeon commented Jun 12, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

philsturgeon commented Jun 12, 2018 via email

Uh oh!

jimschubert commented Jun 12, 2018

Uh oh!

philsturgeon commented Jun 13, 2018 via email

Uh oh!

jmini commented Jun 13, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants