1013833 - Work out a code review process for Metrics work

Status: RESOLVED FIXED

(Mozilla Foundation :: Metrics, task, P2)

Description

[Adam Lofting (:adamlofting)]

Now that some of these apps I've been building are moving into production use, I need to define a process for getting new code reviewed before it’s shipped to production.

Comment 1

[Brian Brennan [:brianloveswords]]

I'm happy to look at stuff, is the development happening on Github?

In general, if things are developed feature-by-feature, what I'd do is create an issue and a new branch related to that issue. When it's ready for review, send a pull request in from the new branch to the master branch, and then ping me (@brianloveswords) in the comments and I can check it out.

In this case I'd imagine there's some code that's already in master that needs a look, so just point me at the repos and I can check them out over the next week.

Comment 2

[Brian Brennan [:brianloveswords]]

Oh I implied this, but I want to elaborate:

Doing the whole "don't commit to master; always make feature branches and send PRs to yourself" is something I highly recommend. I maintain a lot of NPM modules, and most of them don't have co-maintainers (which is something I should fix) and this is a strategy I use on all of those. 

Writing code and reviewing code are two different modes of thinking and by making a PR instead of committing everything straight to master, you give yourself the opportunity to look at your own code in "review mode". I've caught a lot of bugs/bad designs just by giving myself another opportunity to look at my own code with that different perspective.

Comment 3

[Bobby Richter [:secretrobotron]]

+1 to what Brian said about PR's and branches.

We've talked about doing reviews across teams on-demand for different projects in the past, but nothing ever stuck (AFAICT). Hitting mofo-dev with a request isn't a bad start (like you did :) ). There are always keeners like Brian who are willing to help.

Comment 4

[Adam Lofting (:adamlofting)]

Thanks both, this is awesome.

(In reply to Brian Brennan [:brianloveswords] from comment #1)
> I'm happy to look at stuff, is the development happening on Github?

yep :)

> In general, if things are developed feature-by-feature, what I'd do is
> create an issue and a new branch related to that issue. When it's ready for
> review, send a pull request in from the new branch to the master branch, and
> then ping me (@brianloveswords) in the comments and I can check it out.

Thanks, I will make sure to do this.

> In this case I'd imagine there's some code that's already in master that
> needs a look, so just point me at the repos and I can check them out over
> the next week.

This has been through a security review, but a general code review would be greatly appreciated:
https://github.com/mozilla/adhoctribution/

Comment 5

[Adam Lofting (:adamlofting)]

(Thank you both again!)

Comment 6

[Atul Varma [:atul]]

(In reply to Brian Brennan [:brianloveswords] from comment #2)
> Writing code and reviewing code are two different modes of thinking and by
> making a PR instead of committing everything straight to master, you give
> yourself the opportunity to look at your own code in "review mode". I've
> caught a lot of bugs/bad designs just by giving myself another opportunity
> to look at my own code with that different perspective.

Nice, I agree, though I personally tend to do it only with a subset of my changes. It also forces me to explain the rationale for my changes in the PR and/or the commit logs, which is great documentation for the future.

It's also actually best practice #4 in "11 proven practices for more effective, efficient peer code review", one of the very few articles on doing code review I could find on the internet:

http://www.ibm.com/developerworks/rational/library/11-proven-practices-for-peer-review/

Definitely worth a read, IMO!