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

chore: update github PR template [INFENG-600] #9098

Merged
merged 2 commits into from
Apr 5, 2024

Conversation

JComins000
Copy link
Contributor

@JComins000 JComins000 commented Apr 3, 2024

Ticket

INFENG-600

Description

Rewrote some of the PR template; I'm very open to suggestions.

Test Plan

Checklist

  • Changes have been manually QA'd
  • User-facing API changes need the "User-facing API Change" label.
  • Release notes should be added as a separate file under docs/release-notes/.
    See Release Note for details.
  • Licenses should be included for new code which was copied and/or modified from any external code.

@JComins000 JComins000 requested a review from a team as a code owner April 3, 2024 19:41
@cla-bot cla-bot bot added the cla-signed label Apr 3, 2024
Copy link

netlify bot commented Apr 3, 2024

Deploy Preview for determined-ui canceled.

Name Link
🔨 Latest commit d3b49a8
🔍 Latest deploy log https://app.netlify.com/sites/determined-ui/deploys/660f34f5a6bb1700082f9eaf

@JComins000 JComins000 force-pushed the jcom/INFENG-600/PR-template-cleanup branch 4 times, most recently from 133a641 to f37bc1f Compare April 4, 2024 16:18
@JComins000 JComins000 force-pushed the jcom/INFENG-600/PR-template-cleanup branch from f37bc1f to a99caff Compare April 4, 2024 16:22
Copy link
Contributor

@jesse-amano-hpe jesse-amano-hpe left a comment

Choose a reason for hiding this comment

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

Thank you!


Check the "Example Commit Body" for conventional commit semantics.
For breaking changes, please lead with "BREAKING CHANGE:".
Copy link
Contributor

Choose a reason for hiding this comment

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

Previously, this "breaking change" part suggested the PR author to call out the breaking changes in the "description" section (which is now apparently called "commentary").

Now this reads as if you're supposed to put "BREAKING CHANGE: " into the PR title which is not the established process.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point! I'll move that part to the other section. I can keep "description" as well. It looked to be redundant with commentary.

Copy link
Contributor

Choose a reason for hiding this comment

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

ok, yea, I'd personally keep description and get rid of the optional commentary instead of promoting commentary in place of description because I don't see a point in "swapping" these two.

@JComins000 JComins000 force-pushed the jcom/INFENG-600/PR-template-cleanup branch from 2945c3c to d3b49a8 Compare April 4, 2024 23:17
@JComins000 JComins000 changed the title feat: jcom/INFENG-600/PR-template-cleanup chore: update github PR template [INFENG-600] Apr 4, 2024
Copy link
Contributor

@dannysauer dannysauer left a comment

Choose a reason for hiding this comment

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

Just a couple of suggested changes. I'm approving either way, so feel free to accept or reject any of those suggestions before merging. :)

Jira ticket, e.g. "[DET-1234]". When squash-and-merging, copy this directly
into the description field.
## PR TITLE (Commit Body)
When squash-and-merging, copy this directly into the description field.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
When squash-and-merging, copy this directly into the description field.
When squash-merging, this will be used as the commit message.

Users don't need to copy the text in - it'll already be there.

into the description field.
## PR TITLE (Commit Body)
When squash-and-merging, copy this directly into the description field.
Check the "Example Commit Body" for conventional commit semantics.
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe include a link to the actual spec, too?

Suggested change
Check the "Example Commit Body" for conventional commit semantics.
Check the "Example Commit Body" for conventional commit semantics. For more details, https://www.conventionalcommits.org/en/v1.0.0/#summary

-->
## Ticket
<!---
A reference to the Jira ticket or Github issue. e.g. "[DET-1234]".
Copy link
Contributor

Choose a reason for hiding this comment

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

An example of both would likely be useful

Suggested change
A reference to the Jira ticket or Github issue. e.g. "[DET-1234]".
A reference to the Jira ticket or Github issue. e.g. "[DET-1234]" or #123.

@JComins000 JComins000 merged commit 0c6985b into main Apr 5, 2024
67 of 79 checks passed
@JComins000 JComins000 deleted the jcom/INFENG-600/PR-template-cleanup branch April 5, 2024 19:17
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.

4 participants