Skip to content

Conversation

@jimschubert
Copy link
Member

  • Updates the PR template to allow cleaner PR description workflow.
  • Cleans up some of the language/descriptions in check items.

When a single commit PR is opened, GitHub will insert the commit message to the top of the PR template. Users who follow this flow previously needed to move this text to the bottom of the PR text, and remove the placeholder text. It's such common practice for github contributors to enter descriptions in PRs, we can omit the Description placeholder and be less prescriptive about where the description exists.

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\. If contributing template-only or documentation-only changes which will change sample output, be sure to build the project first.
  • Filed the PR against the correct branch: master, 4.1.x, 5.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

(details of the change, additional tests that have been done, reference to the issue for tracking, etc)

* Updates the PR template to allow cleaner PR description workflow.
* Cleans up some of the language/descriptions in check items.

When a single commit PR is opened, GitHub will insert the commit message to the top of the PR template. Users who follow this flow previously needed to move this text to the bottom of the PR text, and remove the placeholder text.  It's such common practice for github contributors to enter descriptions in PRs, we can omit the `Description` placeholder and be less prescriptive about where the description exists.
@jimschubert
Copy link
Member Author

See the unmodified description of this PR's single-commit contribution for an example of what will be resolved by this commit.

Comment on lines +1 to +3
<!-- Enter details of the change here. Include additional tests that have been done, reference to the issue for tracking, etc. -->

<!-- Please check the completed items below -->
Copy link
Member Author

Choose a reason for hiding this comment

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

This HTML comments won't be rendered in the final PR description.

(details of the change, additional tests that have been done, reference to the issue for tracking, etc)

- [ ] If contributing template-only or documentation-only changes which will change sample output, [build the project](https://github.com/OpenAPITools/openapi-generator#14---build-projects) 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).
Copy link
Member

Choose a reason for hiding this comment

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

e.g. for typescript-fetch one should only run bin/typescript-fetch-petstore-all.sh, not bin/openapi3/typescript-fetch-petstore-all.sh

Copy link
Member Author

Choose a reason for hiding this comment

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

If users aren't expected to run the openapi3 script, why does it exist? Does it hurt anything if they do run it?

Copy link
Member

Choose a reason for hiding this comment

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

If users aren't expected to run the openapi3 script, why does it exist?

I don't know, probably for testing purposes?

Does it hurt anything if they do run it?

yes, the body parameter changes its name and some extra inline-files are generated, leading to a failing ensure-samples-up-to-date.sh

Copy link
Member Author

Choose a reason for hiding this comment

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

So, they're generating to the same directories? I think that's an issue if the 2.0 and 3.x outputs aren't the same.

I don't think this warrants an exception in the PR template. We should create an issue to investigate and fix for output parity. That, or generate to separate directories.

Copy link
Member

@wing328 wing328 left a comment

Choose a reason for hiding this comment

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

LGTM. Let's try it.

@wing328 wing328 added this to the 4.2.0 milestone Oct 13, 2019
@wing328 wing328 merged commit 7a22b14 into master Oct 13, 2019
@wing328 wing328 deleted the pr-template branch October 13, 2019 10:03
jimschubert added a commit that referenced this pull request Oct 14, 2019
* master: (78 commits)
  Replaced dashes with underscores in build.gradle files. (#4092)
  [cxf-cdi] use @FormParam for form parameters when it is not Multipart (#4125)
  Corrections to script names (#4135)
  [python] Add missing keywords python (#4134)
  Update PULL_REQUEST_TEMPLATE.md (#4080)
  revert the fix to broken links
  Rename property name from propertyRawName to propertyBaseName (#4124)
  [Go] Fix go.mod and go.sum for 1.13 (#4084)
  [kotlin] add option for non public api (#4089)
  Added new discriminator RawName property to preserve declared discriminator for @JsonTypeInfo annotations (#3320)
  Fix links to other files (#4120)
  [JAVA][JAXRS] Fix parameters validation (#3862)
  Make Resttemplate thread safe by using the withHttpInfo pattern used by many other generated clients (#4049)
  Disabling linting for typescript-fetch (#4110)
  [Kotlin][Client] fix missing curly bracket when the model contains enum property (#4118)
  Fix NPE in Elm path parameter (#4116)
  test aiohttp first (#4117)
  add back ruby client folders
  update petstore samples
  [CLI] Initial implementation for batch generation (#3789)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants