(doc): add pull request template #465

mikemurray · 2018-02-01T21:09:20Z

Adds a pull request template and details to help improve the quality of PR's into core Reaction.

mikemurray · 2018-02-01T21:10:19Z

@spencern @machikoyasuda your thoughts and reviews are welcome

machikoyasuda · 2018-02-02T00:03:11Z

@zenweasel - What do you think about combining #450 issue with this one? This addition adds a checklist for what both the PR author and the Reviewer should do in specific scenarios. You can add anything else in this section: https://github.com/reactioncommerce/reaction-docs/pull/465/files#diff-1b21683f4fb6eabc842c6aa25ccb5411L48

After this PR is merged, we should update the template in https://github.com/reactioncommerce/reaction/blob/master/.github/pull_request_template.md

brent-hoover · 2018-02-02T01:21:56Z

@machikoyasuda Yes, #450 should be combined with this one and we should update the template in the main PR repo. Is this the docs that is linked to when you create a PR?

mikemurray · 2018-02-02T01:24:05Z

@zenweasel Yes, the template and a link back to these docs should be in the PR template.

machikoyasuda · 2018-02-02T01:24:21Z

@zenweasel @mikemurray

The plan is to update https://github.com/reactioncommerce/reaction/blob/master/.github/pull_request_template.md with the content from https://github.com/reactioncommerce/reaction-docs/pull/465/files#diff-1b21683f4fb6eabc842c6aa25ccb5411R34 so that users would see it when they create a PR in reaction core.

We should also link this in https://github.com/reactioncommerce/reaction/blob/master/CONTRIBUTING.md from core too, as it is linked in the README: https://github.com/reactioncommerce/reaction#contribute

So.. post-deploy to do list:

Update Contributing docs on Reaction Docs
Update pull_request_template.md in core with that link from above
Update README.md from core with that link from above

brent-hoover · 2018-02-02T01:29:06Z

@machikoyasuda Just wanted to make sure we were updating this stuff....

machikoyasuda · 2018-02-02T01:36:07Z

@zenweasel That blue contributing link === https://github.com/reactioncommerce/reaction/blob/master/CONTRIBUTING.md

brent-hoover · 2018-02-02T01:43:18Z

@machikoyasuda got it, thanks

spencern

Much more specific than our current template.
Found some phrasings that are difficult to understand a few other issues - capitalizing Reaction, etc.

spencern · 2018-02-03T00:59:52Z

developer/contributing.md

+
+## 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.


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 problem

Isn't this the solution section?

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"

Not sure where best to put this but something like "Failure to follow this template may result in your PR being closed without comment"

spencern · 2018-02-03T01:04:18Z

developer/contributing.md

+
+## 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.


I think this part is really important, but it's a hard to follow (the your, any things)
What do you think about:
Summarize your solution to the problem. Please include short descriptions of any solutions you tested before arriving at your final solution. This will help reviewers know why you decided to solve this problem in this particular way and will speed up the review process.

spencern · 2018-02-03T01:05:31Z

developer/contributing.md

+
+## Breaking changes
+
+- If you have a breaking changes, list them here, otherwise list none.


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.

Breaking change example: reactioncommerce/reaction#3331

reactioncommerce/reaction#3524

spencern · 2018-02-03T01:07:14Z

developer/contributing.md

+
+1. Add steps to test this PR
+1. Be detailed enough that someone can work through it
+1. Avoid being too granular


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

catch 22, we'd have to use the template in a PR before we could link it here

Maybe we can combine Be detailed enough so someone can work through it, while not being too granular.

spencern · 2018-02-03T01:08:01Z

developer/contributing.md

+
+###### 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.


the above template => our template?

point the other to this doc => point to this doc or point the owner to this doc?

spencern · 2018-02-03T01:11:17Z

developer/contributing.md

+
+###### 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.


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?

Might be worth instructing the PR author to ensure that she has migrations or deprecation warnings for any breaking changes that were added.

I would specifically call out Schema changes as an example of a breaking change

spencern · 2018-02-03T01:16:49Z

developer/contributing.md

+
+###### 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.


Reaction should always be capitalized as a proper noun.
Try testing this PR on an existing version of reaction you may already have data for is a little difficult to follow.

maybe something like
`Test applying this patch to an existing install of Reaction with existing users, orders, carts, etc. Specifically, test any parts of the app where the breaking change is involved and any data set that is involved in a migration.

spencern · 2018-02-03T01:17:51Z

developer/contributing.md

+
+###### 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.


before issuing an approval or a request for changes => before approving or requesting changes`

brent-hoover

Just a few notes plus I would suggest reorganizing the last section to keep author and reviewer lists separate.

brent-hoover · 2018-02-06T03:38:57Z

developer/contributing.md

+
+## 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.


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"

brent-hoover · 2018-02-06T03:40:12Z

developer/contributing.md

+
+## 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.


Not sure where best to put this but something like "Failure to follow this template may result in your PR being closed without comment"

brent-hoover · 2018-02-06T03:46:06Z

developer/contributing.md

+
+###### Reviewer
+
+Read over the original ticket, and understand the impact this PR will have on the codebase. If the PR doesn't follow the our template, reject and point the author of the PR to this doc.


I'm confused by this section. Can we just have one PR Author and one PR Review section rather than these alternating ones? Am I the only one bothered by this? One is a list of drop-dead requirements and one is a list of "things to consider", I'm not sure if they are really equivalent?

Somewhere (I can't tell where in this current format) we need to have a checklist of things code reviewers should consider.

Performance (does the code use best practices for minimizing the load on client and server hardware)

Concurrency (are data transactions multi-user safe? Am I prevented from accidentally overwriting someone else changes?)

Security (are permissions being checked everywhere appropriate? Are error messages being sanitized before they reach the client. Are we checking inputs properly wherever we accept client input?)

These are not a comprehensive lists. But we should start compiling them.

@zenweasel agreed, though those things should not be in the template, they should be in the docs. I would not consider that to be a blocking issue for this PR but something worth adding.

brent-hoover · 2018-02-06T03:46:20Z

developer/contributing.md

+###### 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.
+


typo: "of ideas"

brent-hoover · 2018-02-06T03:48:39Z

developer/contributing.md

+
+###### 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.


"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"

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

brent-hoover · 2018-02-06T04:00:42Z

developer/contributing.md

+
+###### PR author
+
+Summarize your solution to the problem. Please include short descriptions of any solutions you tested before arriving at your final solution. This will help reviewers know why you decided to solve this problem in this particular way and will speed up the review process.


Maybe just "Summarize how you fixed the problem. Please include short descriptions of other things you tried before arriving at your final solution. "
Just to cut down on the repetition of the word "solution"?

"If you have introduced new dependencies, explain why and how you determined that said dependency was the best solution to the problem."

brent-hoover · 2018-02-06T04:01:19Z

developer/contributing.md

+
+###### 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.


typo: "determine the path"

Updated based on review requests

(doc): add pull request template

f5d0d3f

mikemurray requested review from machikoyasuda and spencern February 1, 2018 21:09

machikoyasuda approved these changes Feb 2, 2018

View reviewed changes

machikoyasuda requested a review from brent-hoover February 2, 2018 00:03

spencern suggested changes Feb 3, 2018

View reviewed changes

mikemurray added 2 commits February 5, 2018 09:58

(doc): update reviewer instructions for clarity

d624695

(doc): update author solution description

a8a4da5

brent-hoover previously requested changes Feb 6, 2018

View reviewed changes

spencern added 2 commits February 8, 2018 11:52

Remove CLA review section because we no longer have a CLA

c77c988

A few updates to Mike's PR template and instructions.

bad61a4

spencern approved these changes Feb 8, 2018

View reviewed changes

Merge branch 'master' into mm-update-pr-guide

5a00df3

chore: linting

475d279

machikoyasuda merged commit 475d279 into master Feb 8, 2018

machikoyasuda deleted the mm-update-pr-guide branch February 8, 2018 23:52

spencern mentioned this pull request Feb 9, 2018

Update pull request template reactioncommerce/reaction#3687

Merged


		## 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.


		## Breaking changes

		- If you have a breaking changes, list them here, otherwise list none.


		###### 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.


		###### 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.


		###### 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.


		###### 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.

		###### 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.


		###### 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.


		###### PR author

		Summarize your solution to the problem. Please include short descriptions of any solutions you tested before arriving at your final solution. This will help reviewers know why you decided to solve this problem in this particular way and will speed up the review process.


		###### 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.

(doc): add pull request template #465

(doc): add pull request template #465

Conversation

mikemurray commented Feb 1, 2018

mikemurray commented Feb 1, 2018

machikoyasuda commented Feb 2, 2018

brent-hoover commented Feb 2, 2018

mikemurray commented Feb 2, 2018

machikoyasuda commented Feb 2, 2018 • edited Loading

brent-hoover commented Feb 2, 2018

machikoyasuda commented Feb 2, 2018

brent-hoover commented Feb 2, 2018

spencern left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brent-hoover left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

machikoyasuda commented Feb 2, 2018 •

edited

Loading