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.

Sign up for GitHub

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

Document code review process at OKI #6

Merged
merged 4 commits into from
Feb 17, 2016
Merged

Document code review process at OKI #6

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
aa0c440
Code reviews
pwalsh Feb 15, 2016
28ffd69
Add small text
pwalsh Feb 15, 2016
e5f3002
Address comments from reviewers.
pwalsh Feb 16, 2016
f3e421f
Fix typo
brew Feb 17, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Add small text
  • Loading branch information
@pwalsh
pwalsh committed Feb 15, 2016
commit 28ffd69823abeaaca9e010d50e7d4845ae0a2f7e
2 changes: 1 addition & 1 deletion sections/reviews.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ Submitting code for review should follow these steps:
3. Each feature or fix branch should be focused on a discrete unit of work.
4. When your unit of work is complete, submit a pull request against the `master` branch on `origin`.
Copy link
Member

Choose a reason for hiding this comment

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

Make sure the PR has a description explaining the nature of the change, in technical terms.

i.e. not 'fixing the multi-view feature' but 'adding exception handling to the DB migrations'

(no, the name of the branch or title of the commit are usually not enough...)

5. Wait for the CI server to run, validating your tests pass on all target environments. If you do not have tests, or have not configured CI, do that immediately before proceeding.
6. If CI is green, ask a colleague to review your pull request. This preferably will be a colleague who is not working on your project.
6. If CI is green, ask a colleague to review your pull request (we will automate this in the near future). This preferably will be a colleague who is not working on your project.
Copy link
Member

Choose a reason for hiding this comment

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

Suggestion: Let's have a CR channel in slack which you could post PR urls in there, and whoever's available can take the review (better than all the coordination described here IMO).

Copy link
Member

Choose a reason for hiding this comment

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

(we could have a slackbot to do that, naturally, reducing a little more boilerplate)

Copy link
Member Author

Choose a reason for hiding this comment

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

We will do that slackbot/automation.

7. Address any questions from the code review. Sometimes this will involving refactoring, other times it will just mean answering questions.
8. When the reviewer indicates that the pull request is ready to merge, you may merge into `master`.
Copy link
Member

Choose a reason for hiding this comment

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

So you merge your own PR? Or is the reviewer who does it?

Copy link
Member Author

Choose a reason for hiding this comment

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

As long as the reviewer indicates that the pull request is ready, I don't think it matters who "clicks the button". The reason that I do not expect the reviewer to actually merge is because the reviewer is not necessarily working on the codebase and so perhaps in these circumstances it is better to let the developer chose the time for merge.

We can change it to ask reviewer to always merge, on the assumption that the developer is expecting this to happen when the code is approved.


Expand Down