-
Notifications
You must be signed in to change notification settings - Fork 187
Add PR template #2273
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
base: master
Are you sure you want to change the base?
Add PR template #2273
Changes from 4 commits
0676439
8328dab
39511c5
63f0e2a
e8a641c
3d5f59c
44a703c
ee885e0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -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? | ||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||
| ## If not already covered by an issue (specified below), what is the motivation for the change(s) in this PR? | |
| ## Motivation |
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not entirely necessary. Github allows you to link an issue on the sidebar
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is kind of a rehash of the description. Can likely omit it, or else simplify it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| ## 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair enough. This has dubious utility anyway since QD reviews commit history himself for writing changelogs.
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm also not sure how many people know "when appropriate" is for adding tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would say to put as much as possible in the contribution guidelines and avoid putting it in the template.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.