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

Update PR template #2791

Merged
merged 5 commits into from
Sep 29, 2023
Merged

Conversation

celinval
Copy link
Contributor

Change Description

Change the template to be more concise to address the team's concern that the template was too verbose.

Call-outs

  1. I don't think we need the disclaimer at the bottom, but I'm keeping it for now.
  2. BTW, I do like the call-outs to provide context to the PR, but I believe we can leave it to the author's discretion.

Note: By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 and MIT licenses.

Change the template to be more concise.

Note: I don't know if we need the disclaimer at the bottom, so I'm keeping it for now
@celinval celinval requested a review from a team as a code owner September 26, 2023 01:09
Remove the sections and use a quote block instead of comments to provide instructions that should be removed.
Copy link
Contributor

@zhassan-aws zhassan-aws left a comment

Choose a reason for hiding this comment

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

IMHO, the current template is clearer, and is more convenient to fill out because of the structure it provides. It could use some cleanup though (e.g. removing the checkboxes list, and perhaps the testing section). Would that make the commit message too cumbersome?

.github/PULL_REQUEST_TEMPLATE.md Outdated Show resolved Hide resolved
.github/PULL_REQUEST_TEMPLATE.md Outdated Show resolved Hide resolved
.github/PULL_REQUEST_TEMPLATE.md Outdated Show resolved Hide resolved
@adpaco-aws
Copy link
Contributor

Would that make the commit message too cumbersome?

In my opinion, they'll be a bit harder to read. But that's not the only problem: including structure will make them more difficult to distinguish because they'll include repeated content (for example, think about how git log would look like).

Should we update our development documentation to clearly state that we'll be using the PR description as the commit message? And what parts to remove before doing so, too.

Co-authored-by: Zyad Hassan <88045115+zhassan-aws@users.noreply.github.com>
Copy link
Contributor

@JustusAdam JustusAdam left a comment

Choose a reason for hiding this comment

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

Seems reasonable

@celinval
Copy link
Contributor Author

I don't think there's a perfect solution here, but this is also a small problem that I don't think it's worth spending too much time. The goal is to reduce unnecessary burden and ensure that our git history captures the right amount of information.

Thank you for all the feedback! I'm going ahead with the current proposal, and I think we can adjust later.

@celinval celinval enabled auto-merge (squash) September 29, 2023 17:37
@celinval celinval merged commit ed671fc into model-checking:main Sep 29, 2023
13 checks passed
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.

5 participants