Add PR template #2273
Add PR template #2273
Conversation
|
ah that would be nice |
ThomasJRyan
left a comment
ThomasJRyan
left a comment
There was a problem hiding this comment.
The whole thing is a bit verbose and overwhelming for a contributor. Consider these changes
.github/pull_request_template.md
Outdated
| @@ -0,0 +1,26 @@ | |||
| ## What does this PR change? | |||
There was a problem hiding this comment.
| ## What does this PR change? | |
| ## Description |
.github/pull_request_template.md
Outdated
| @@ -0,0 +1,26 @@ | |||
| ## What does this PR change? | |||
|
|
|||
| ## If not already covered by an issue (specified below), what is the motivation for the change(s) in this PR? | |||
There was a problem hiding this comment.
| ## If not already covered by an issue (specified below), what is the motivation for the change(s) in this PR? | |
| ## Motivation |
.github/pull_request_template.md
Outdated
| ## What issues does this PR resolve? (Add as many as are applicable) | ||
| ### Example: Fixes #12345 | ||
| - Fixes # | ||
| - Fixes # |
There was a problem hiding this comment.
Not entirely necessary. Github allows you to link an issue on the sidebar
There was a problem hiding this comment.
I understand you can link it from the sidebar. It's missed often though. Or people submit changes without having first submitted an issue describing the thing they're trying to fix/add.
.github/pull_request_template.md
Outdated
| ## What design decisions did you make to develop your solution ? Why did you make those choices? | ||
| - If this change required refactoring existing code, explain why it needed to be refactored. What limitation are you resolving with the refactor? |
There was a problem hiding this comment.
This is kind of a rehash of the description. Can likely omit it, or else simplify it
There was a problem hiding this comment.
The description isn't verbose enough often times.
I'll often see "explain why you did this the way you did it" type questions from QD and I hoped a question like this up front could trim down on how many review cycles are necessary as a description like this could provide more useful information up front.
.github/pull_request_template.md
Outdated
| ## What design decisions did you make to develop your solution ? Why did you make those choices? | ||
| - If this change required refactoring existing code, explain why it needed to be refactored. What limitation are you resolving with the refactor? | ||
|
|
||
| ## If this change is something the users will notice, please write a one-sentence description of this change so it may be used in a changelog: |
There was a problem hiding this comment.
| ## If this change is something the users will notice, please write a one-sentence description of this change so it may be used in a changelog: | |
| ## Changelog |
Though admittedly I would opt to omit it. Squashed commit messages seem to be the preferred method of getting changelog content
There was a problem hiding this comment.
Fair enough. This has dubious utility anyway since QD reviews commit history himself for writing changelogs.
.github/pull_request_template.md
Outdated
| ## Checklist before submitting the PR: | ||
| - [ ] Please read the contributing guidelines document and ensure your changes are in compliance with those guidelines. | ||
| - [ ] Are there any open pull requests that resolve the same issue? If so, mention them in this PR. Multiple approaches are welcomed as the one that is liked most will be selected. | ||
| - [ ] Please run the cubyz formatter on your changes to ensure formatting consistency | ||
| - The CI will catch this if you don't | ||
| - [ ] Make sure your PR is named appropriately to describe the change briefly. No "fix #12345" etc, please. | ||
| - [ ] Did you remove an old item/block/biome/etc? Please add a migration in the appropriate `_migrations.zig.zon` to smoothly migrate existing worldsto your new change. | ||
| - [ ] Have you tested your change? | ||
| - [ ] Make sure the game opens and you can join a world | ||
| - [ ] Analyze your change and determine which features it impacts. Make sure your change does not cause those features to break. | ||
| - [ ] Did you modify or add any static functions? (i.e. data in -> data out, no out-of-scope variables touched) If so, please write a test for those functions you've changed/added. |
There was a problem hiding this comment.
This checklist is extremely verbose and will be glazed over by the average contributor. Try to keep sentences short and limit your checks to no more than five.
| ## Checklist before submitting the PR: | |
| - [ ] Please read the contributing guidelines document and ensure your changes are in compliance with those guidelines. | |
| - [ ] Are there any open pull requests that resolve the same issue? If so, mention them in this PR. Multiple approaches are welcomed as the one that is liked most will be selected. | |
| - [ ] Please run the cubyz formatter on your changes to ensure formatting consistency | |
| - The CI will catch this if you don't | |
| - [ ] Make sure your PR is named appropriately to describe the change briefly. No "fix #12345" etc, please. | |
| - [ ] Did you remove an old item/block/biome/etc? Please add a migration in the appropriate `_migrations.zig.zon` to smoothly migrate existing worldsto your new change. | |
| - [ ] Have you tested your change? | |
| - [ ] Make sure the game opens and you can join a world | |
| - [ ] Analyze your change and determine which features it impacts. Make sure your change does not cause those features to break. | |
| - [ ] Did you modify or add any static functions? (i.e. data in -> data out, no out-of-scope variables touched) If so, please write a test for those functions you've changed/added. | |
| ## Checklist before submitting the PR: | |
| - [ ] I've read the [contribution guidelines](https://github.com/PixelGuys/Cubyz/blob/master/docs/CONTRIBUTING.md) | |
| - [ ] I've added a migration if necessary | |
| - [ ] I've added tests where appropriate |
There was a problem hiding this comment.
I don't think we can state "a migration if necessary" without describing why they'd need it and where it is in the first place. Maybe this belongs in contribution guidelines.
There was a problem hiding this comment.
I'm also not sure how many people know "when appropriate" is for adding tests
There was a problem hiding this comment.
I would say to put as much as possible in the contribution guidelines and avoid putting it in the template.
5 participants
I'm hoping this can help with guiding people to create PRs that are easier to review as they've provided more information from the start instead of us needing to fish for more information.
We can adjust wording and add/remove lines in this template as desired before merge.