(doc): add pull request template #465
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -31,6 +31,84 @@ Start small and open up a dialogue with us. This will help to get your contribut | |
| - Contributors should review the CLA. | ||
| - Code reviewed before merge acceptance. | ||
|
|
||
| ### Pull request template | ||
|
|
||
| ``` | ||
| Resolves #issueNumber | ||
| Impact breaking|minor|bug|style|refactor | ||
|
|
||
| ## Issue | ||
|
|
||
| Description of the issue this PR is solving, why it's happening and how it can be reproduced. This may differ from the original ticket as you now have more information at your disposal. | ||
|
|
||
| ## Solution | ||
|
|
||
| Summarize the your solution to the problem, as well as any things you may have tried along the way that may not have worked. | ||
|
There was a problem hiding this comment. I think this part is really important, but it's a hard to follow ( |
||
|
|
||
| ## Breaking changes | ||
|
|
||
| - If you have a breaking changes, list them here, otherwise list none. | ||
|
There was a problem hiding this comment. We may need to give some examples (here or linked) as to what constitutes a breaking change. I'm not sure we have this documented somewhere, but it would be valuable. There was a problem hiding this comment. Breaking change example: reactioncommerce/reaction#3331 There was a problem hiding this comment. |
||
|
|
||
| ## Testing | ||
|
|
||
| 1. Add steps to test this PR | ||
| 1. Be detailed enough that someone can work through it | ||
| 1. Avoid being too granular | ||
|
There was a problem hiding this comment. Would be nice to be able to link to a few great example PRs as well, if not from the template itself, then from the contributing guidelines There was a problem hiding this comment. catch 22, we'd have to use the template in a PR before we could link it here There was a problem hiding this comment. Maybe we can combine |
||
| ``` | ||
|
|
||
| #### Template sections | ||
|
|
||
| ##### Top section | ||
|
|
||
| ###### PR author | ||
|
|
||
| The first line of every PR should have `Resolves #ticketNumber`. | ||
| The second line of every PR should have `Impact breaking|minor|bug|style|refactor`. | ||
|
|
||
| ###### Reviewer | ||
|
|
||
| Read over the original ticket, and understand the impact this PR will have on the codebase. If the PR doesn't follow the above template, reject and point the other to this doc. | ||
|
There was a problem hiding this comment.
|
||
|
|
||
| ##### Issue | ||
|
|
||
| ###### PR author | ||
|
|
||
| Describe the issue this PR is solving with the knowledge you've gained by fixing it. This may differ from the original ticket as you now have more information at your disposal. | ||
|
|
||
| ###### Reviewer | ||
|
|
||
| Use this information as the basis for your review. If something is not clear here, Reject the PR and ask for clarity. While the original issue may have useful information, the PR should be the most up to date representation if ideas. | ||
|
|
||
|
|
||
| ##### Solution | ||
|
|
||
| ###### PR author | ||
|
|
||
| Summarize the your solution to the problem, as well as any other solutions you may have tried along the way that may not have worked. | ||
|
|
||
| ###### Reviewer | ||
|
|
||
| Use this information to help determine path to test this PR. Research any included packages or techniques that may have been used that you're not familiar with. Ask questions if you're confused. | ||
|
There was a problem hiding this comment. typo: "determine the path" |
||
|
|
||
| ##### Breaking changes | ||
|
|
||
| ###### PR author | ||
|
|
||
| List any breaking changes that come with this PR. If you have none, then simply put `none`. Examples of breaking changes include changing file names, moving files around, deleting files, renaming functions or exports, or significantly changing code to where previous versions of the app may not work as expected. | ||
|
There was a problem hiding this comment. This may be a good enough example list for now. Should we include this in the template? Not sure how verbose the template should be. @machikoyasuda any thoughts? There was a problem hiding this comment. Might be worth instructing the PR author to ensure that she has migrations or deprecation warnings for any breaking changes that were added. There was a problem hiding this comment. I would specifically call out Schema changes as an example of a breaking change |
||
|
|
||
| ###### Reviewer | ||
|
|
||
| If there are breaking changes, make you'll need to make sure any necessary migrations or guards have been put into place so existing apps do not break. Try testing this PR on an existing version of reaction you may already have data for. | ||
|
There was a problem hiding this comment.
maybe something like |
||
|
|
||
| ##### Testing | ||
|
|
||
| ###### PR author | ||
|
|
||
| List the steps needed for testing your change in this section. Assume that testers already know how to start the app, and do the basic setup tasks. Beyond that, the steps should guide users through the feature or fix you've implemented in this PR. Include additional steps if multiple paths are available. | ||
|
There was a problem hiding this comment. "Also point out areas where your change may have broken existing functionality and list those areas as well. A good test plan created by the Author is one of the keys to maintaining quality and it's in your interest to make your test plan as complete as possible" There was a problem hiding this comment. In super big letters "If your PR fails the basic test instructions your PR may be closed and may receive no further consideration". In other words, if you give me one thing to test on your PR, and even that doesn't work, I am not wasting any more time on your PR |
||
|
|
||
| ###### Reviewer | ||
|
|
||
| Run through the author's steps to verify that it works as they've tested it. Then run through the app on your own as you would test it. Run through the app as many times as you feel comfortable before issuing an approval or a request for changes. | ||
|
There was a problem hiding this comment.
|
||
|
|
||
| ### Here's what to expect when you make a pull request to Reaction | ||
|
|
||
| As soon as pull requests are pushed, automated test are run to ensure: | ||
|
|
||
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 personally like the Oxford comma here.
why it's happening, and how it can be reproduced.Maybe add a third sentence here to encourage additional information being included?
A great PR will include additional information gathered during the process of resolving the ticket that might be helpful to reviewers or other users who might encounter the same problemThere 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.
Isn't this the solution section?
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.
We should specifically say something like "you must include all information here so that the PR reviewer can review this PR without reading the original issue". So no "see issues for test instructions"
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 sure where best to put this but something like "Failure to follow this template may result in your PR being closed without comment"