Skip to content

Conversation

@jimschubert
Copy link
Member

@jimschubert jimschubert commented Jan 7, 2020

Looking for feedback on adding this to docs. It's a lot of additional text.

ul elements will be single-column in github, but displayed as expected in our doc site.

image

PR checklist

  • Read the contribution guidelines.
  • If contributing template-only or documentation-only changes which will change sample output, build the project before.
  • Run the shell script(s) under ./bin/ (or Windows batch scripts under.\bin\windows) to update Petstore samples related to your fix. This is important, as CI jobs will verify all generator outputs of your HEAD commit, and these must match the expectations made by your contribution. You only need to run ./bin/{LANG}-petstore.sh, ./bin/openapi3/{LANG}-petstore.sh if updating the code or mustache templates for a language ({LANG}) (e.g. php, ruby, python, etc).
  • File the PR against the correct branch: master, 4.3.x, 5.0.x. Default: master.
  • Copy the technical committee to review the pull request if your PR is targeting a particular programming language.

@jimschubert
Copy link
Member Author

cc @OpenAPITools/generator-core-team @OpenAPITools/openapi-generator-collaborators

@spacether
Copy link
Contributor

spacether commented Jan 8, 2020

@jimschubert could you also add:

  • the additional-properties options that are available to each generator
  • include an sample that shows how to use two or more together, or a link to that

@jimschubert
Copy link
Member Author

@spacether additional properties aren't evaluated until processOpts (the generate command). This would need to be done in some --dry-run option in the generate command (along with supporting files).

@jimschubert
Copy link
Member Author

I added more details to #1811 about the options we can only get from the generate command.

@jimschubert jimschubert merged commit a2532cc into OpenAPITools:master Jan 9, 2020
@jimschubert jimschubert deleted the doc-full-details branch January 9, 2020 02:54
@spacether
Copy link
Contributor

@jimschubert
Copy link
Member Author

@spacether what you've linked to is CLI options which were already documented before this PR (the CONFIG OPTIONS header). See: https://github.com/OpenAPITools/openapi-generator/blob/master/docs/generators/python.md

Additional Properties are different. It's a map of options which holds values usually including CLI options, plus others that can only be determined after the generate command and subsequently processOpts.
Compare the package name CLI option just above the line you've linked and this one in processOpts:

if (additionalProperties.containsKey(CodegenConstants.PACKAGE_NAME)) {

For better examples of why it would only be helpful to dump these from generate, see the following generators which set additionalProperties and/or supportingFiles based on user inputs:

Also, another example of how vendor extensions (those we apply for our tooling) will only be known after the generate command:

There may be opportunities for CLI options, although we do already output defaults and the CLI option type itself outputs available enum information. I would think any confusion here might be generator specific, like if a generator has a set of values but doesn't define them as an enum but rather a freeform string.

I apologise for any confusion, but I didn't realize folks considered CLI options to also be additional properties. I think it's only coincidental that they usually end up there.

@spacether
Copy link
Contributor

Thank you for explaining that. I didn't understand how all of those were related.

@spacether
Copy link
Contributor

Also people don't necessarily know about this link that you pointed us to.
https://github.com/OpenAPITools/openapi-generator/blob/master/docs/generators/python.md
The new doc page that you are generating also looks like it would benefit from including this same info ir linking to the correct md file

@jimschubert
Copy link
Member Author

Forgot to mention, we do already document how to pass multiple additional properties.. see https://openapi-generator.tech/docs/usage#additional-properties

I don't know if it makes sense to double-document this, or to add a link to this on the config page. These are generator specific details, while how to define additional properties is tool specific. That is, it's different for CLI, Maven plugin, Gradle Plugin, Online, and the Bazel Plugin. I think we can safely assume that people can find those docs easily via the doc site. If people are navigating docs in GitHub, usage.md is in the parent directory from the auto-generated docs.

If we cross-link, I'm pretty sure it won't work in docusaurus unless it's a full http link, and that will introduce weird navigation for those using GitHub.

@jimschubert
Copy link
Member Author

@spacether

Also people don't necessarily know about this link that you pointed us to.

I'm not sure how to solve for that. The project follows a pretty standard pattern of putting documentation under the docs folder, and a single generators folder there (plus other docs at the root). This generators folder has a README which lists the generators for GitHub navigation.

Do you mean people using CLI don't know to look here? This is why I've added the same info as above to the config-help command earlier this week. It's behind a --full-details flag for now, so it won't break anyone who might be parsing the output (for whatever reason).

The new doc page that you are generating also looks like it would benefit from including this same info ir linking to the correct md file

I don't understand the comment. I'll assume you mean the doc page in the screenshot should link to this markdown page? Our doc site, which is linked in the README and GitHub repo description, uses docusaurus to build the site from the markdown under docs. It rebuilds the site on every push to master, and what you see in the python.md file I've linked gets much prettier after build: https://openapi-generator.tech/docs/generators/python

It's this site generation which is the ultimate reason for those markdown files to exist. However, for docusaurus to build it's graph of cross-links, the markdown files have to be registered in a specific way. These generated markdown files can't be registered in the expected way because docusaurus isn't great at files in nested directories. So, to link from these back to other doc pages, it would have to be the full doc site URL rather than a relative link (which works in all other markdown pages since they're not nested in directories). That's the background for my above comment about making for weird navigation in GitHub.

I hope that clarifies (and was on point for your comment). As always, I'm open for suggestions to improve things.

@spacether
Copy link
Contributor

spacether commented Jan 9, 2020

Thank you for clarifying and explaining this.
Sorry for the back and forth, I was confused an misunderstood what this PR was doing.
I didn't know that our md files are being used to autogenerate files at https://openapi-generator.tech/docs/
Understanding this now, I see that Python.md includes config options, and that we are updating that same generator markdown file.
Mistakenly I thought that we we editing other files because the screenshot looked so different from what I knew and the description didn't say we are updating each generator's markdown file, which in turn autogenerates files at https://openapi-generator.tech/docs/generators/python
This includes the data that I want our users to have. On a generator page I want them to be able to answer questions like:

  • What reserved words are in this generator
  • What CLI options can I use with this generator
  • [FUTURE] what Swagger/Openapi features are supported in this generator

Thank you for your patience and the thorough explanation of the changes that you made and how our repo and the docs site works

jimschubert added a commit to jimschubert/openapi-generator that referenced this pull request Jan 11, 2020
* master: (187 commits)
  [core] Initial FeatureSet structures and definitions (OpenAPITools#3614)
  Add Cisco to the user list (OpenAPITools#4971)
  comment out php slim4 in ensure-up-to-date
  update samples
  [Python] Allow models to have properties of type self (OpenAPITools#4888)
  Add npmRepository option to javascript generators (OpenAPITools#4956)
  [Slim4] Add ref support to Data Mocker (OpenAPITools#4932)
  Fix auto-labeler for jax-rs (OpenAPITools#4943)
  [doc] full generator details (OpenAPITools#4941)
  comment out python flask 2 test (OpenAPITools#4949)
  [jaxrs-spec][quarkus] update to version 1.1.1.Final (OpenAPITools#4935)
  [cli] Full config help details (OpenAPITools#4928)
  Add RequestFile to typescript-node model template (OpenAPITools#4903)
  [csharp] enum suffix changes enumValueNameSuffix to enumValueSuffix (OpenAPITools#4927)
  [C#] allow customization of generated enum suffixes (OpenAPITools#4301)
  [Kotlin] Correct isInherited flag for Kotlin generators (OpenAPITools#4254)
  [Rust Server] Fix panic handling headers (OpenAPITools#4877)
  Initial CODEOWNERS (OpenAPITools#4924)
  [scala] Support for Set when array has uniqueItems=true (OpenAPITools#4926)
  remove nodejs server samples, scripts (OpenAPITools#4919)
  ...
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.

2 participants