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

[core] Move overwrite ownership to SupportingFile #5036

Merged
merged 3 commits into from
Jan 29, 2020

Conversation

jimschubert
Copy link
Member

@jimschubert jimschubert commented Jan 18, 2020

Previously, there was a writeOptional method in DefaultCodegen which
allowed *Codegen instances to immediately write out a supporting file if
it did not exist. This would allow a codegen implementation to skip
user-facing options such as definitions in .openapi-codegen-ignore, the
"supportingFiles" system property, and support for the experimental
handlebars templating engine. While our implementation only modified the
supportingFiles list conditionally, it added confusion as it seemed to
imply that file writes were somewhat the responsibility of
DefaultCodgen (it's DefaultGenerator which handles file manipulation).

This commit moves the definition of whether a file supports overwriting
existing files into the SupportingFile type itself, allowing that
functionality to be determined at time-of-write rather than
time-of-definition. This would allow us, for example, to dump the list
of files which would be generated using a --dry-run option or similar.

This will be a breaking change for anyone who has extended
DefaultCodegen and called "writeOptional". The path to migrate is to add
the SupportingFile to the supportingFiles list and chain the method call
.doNotOverwrite() on the instance.

This has the added benefit of clarifying this behavior, considering the
write behavior wasn't previously "optional" writes but optionally
defining the list of supportingFiles based on the state of the file
system.

Fixes #4582

This is both a bug fix (due to previous behavior of potentially circumventing user options) and a breaking change (due to workflow options exposed to users via DefaultCodegen rather than DefaultGenerator).

cc @OpenAPITools/generator-core-team

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.

@auto-labeler
Copy link

auto-labeler bot commented Jan 18, 2020

👍 Thanks for opening this issue!
🏷 I have applied any labels matching special text in your issue.

The team will review the labels and make any necessary changes.

@jimschubert
Copy link
Member Author

jimschubert commented Jan 18, 2020

I should clarify:

Our writeOptional implementation would previously only add the file to the supportingFiles list if the file didn't exist. Being a method in DefaultCodegen, there's nothing that would have prevented someone from writing immediately (like if someone said "Why isn't writeOptional actually writing?").

There's technically nothing stopping users from circumventing DefaultGenerator's workflow and writing files whenever they want from a Codegen implementation, but this aims to clarify ownership of that behavior as before the SupportingFile would sometimes be in the list of supporting flies and sometimes would not be in the list.

Previously, there was a writeOptional method in DefaultCodegen which
allowed *Codegen instances to immediately write out a supporting file if
it did not exist. This would allow a codegen implementation to skip
user-facing options such as definitions in .openapi-codegen-ignore, the
"supportingFiles" system property, and support for the experimental
handlebars templating engine. While our implementation only modified the
supportingFiles list conditionally, it added confusion as it seemed to
imply that file writes were somewhat the responsibility of
DefaultCodgen (it's DefaultGenerator which handles file manipulation).

This commit moves the definition of whether a file supports overwriting
existing files into the SupportingFile type itself, allowing that
functionality to be determined at time-of-write rather than
time-of-definition. This would allow us, for example, to dump the list
of files which would be generated using a --dry-run option or similar.

This will be a breaking change for anyone who has extended
DefaultCodegen and called "writeOptional". The path to migrate is to add
the SupportingFile to the supportingFiles list and chain the method call
`.doNotOverwrite()` on the instance.

This has the added benefit of clarifying this behavior, considering the
write behavior wasn't previously "optional" writes but optionally
defining the list of supportingFiles based on the state of the file
system.
@jimschubert
Copy link
Member Author

I've squashed and force-pushed, trying to add clarification to the commit comment and PR description about my concerns with the previous behavior.

* 5.0.x: (92 commits)
  [Rust Server] Handle array of objects inside an object correctly (#5044)
  [Rust Server] Sanitise enum values (#5042)
  [Rust Server] Add support for primitive arrays (#5041)
  [elm] Fix duplicate coder names (#5100)
  [elm] enum items and parameters (#5051)
  [elm] Fix decoding empty operation responses (#5055)
  resolve merge conflicts in VERSION files
  undo restoring ElmClientCodegenTest.java
  fix merge conflicts
  [dart-dio] Various fixes (#5027)
  Add java code comments based on explanation from Jim Schubert (#5048)
  Execute ./bin/openapi3/go-experimental-petstore.sh script (#5049)
  [docs] Sorted doc outputs and clean up duplicated CliOptions (#5046)
  Add docs/installation.md and docsite index to version update script (#5037)
  [gradle] consistent use of maven url in gradle files (#5045)
  Use HTTPS Maven URL in Kotlin meta generator (#5043)
  [Python] fix numeric enum in python flask, aiohttp (#5019)
  add multipleOf to various codegen classes (#5021)
  Add a link to a youtube video on Pulp and OpenAPI Generator (#5038)
  [scripts] Remove misspelled OPENAPI_GENERATOR_DOWLOAD_CACHE_DIR (#5035)
  ...
@jimschubert jimschubert merged commit 607f5a1 into 5.0.x Jan 29, 2020
@jimschubert jimschubert deleted the optional-supporting-files branch January 29, 2020 23:26
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.

1 participant