[okfn-labs] Code review exchange

Wed Jun 5 14:07:27 UTC 2013

On Wed, Jun 5, 2013 at 8:00 AM, Dan <daniel at ohuiginn.net> wrote:

> That's a GREAT idea. You can add me to your list of willing reviewers. I
> know python & js well, though I have no knowledge (yet) of the
> openspending codebase.
>
> From the perspective of a reviewer, what I would need to know is:
>  - what coding standards you have, or other expectations of the code
> (test coverage? comments? namespace management?)
>  - what is the practical workflow for review? [add comments to github
> pull requests?]
>  - how do I signal when I do/don't have time to contribute? [maybe this
> works if it's just "review a pull request when you have time"]
>
> Perhaps a (very short) "how to be a reviewer" document would be useful.
>

The web site doesn't appear to have a link to Github, but the stuff to be
reviewed seems to be here:
https://github.com/openspending/openspending/pulls

I couldn't find any documentation of the process or standards in the wiki
or source tree.

Even simple things like standard terminology are helpful.  The first one I
looked at to get a feel for how things worked had a comment of "This is a
no-brainer." which is apparently a sign of approval because it got merged.
I'd suggest +1 or Google's LGTM or some other standard approval phrase
that's unambiguous.

In addition to Dan's (very good) questions, I'd add:

- What factors determine when a change is high enough risk to require
depolyment to the staging server?

Tom

>
> best,
> Dan
>
> On 05/06/13 12:31, Tryggvi Björgvinsson wrote:
> > Hi all,
> >
> > OpenSpending is a platform that a lot of users (other sites) rely on so
> > we care deeply about quality assurance. One of the things we do as part
> > of QA is code review. A developer should never merge his/her changes
> > into master unless somebody else has looked over the changes and given
> > them a "go".
> >
> > For bigger changes we can also deploy the changes to our staging server
> > so code reviewers can test them on a live site as well.
> >
> > Now, this makes the contribution process a bit longer. You contribute,
> > then you wait, then somebody reviews when that developer has time and
> > your changes (hopefully) get committed. If you need to make some more
> > changes you have to go back in line after you commit them.
> >
> > Therefore we need many code reviewers to keep the process fast. We do
> > not make any requirements of technical knowledge about OpenSpending but
> > the code reviewer should be proficient in both Python and Javascript
> > (and web development in general). The job of the code reviewer is just
> > to ask a lot of questions if there's any doubt as to why things are
> > implemented in a certain way.
> >
> > So I want to make a proposal. Let's do "collaborative QA", we do code
> > review exchange. Who on this list are willing to become a code reviewers
> > for OpenSpending? In return I will become a code reviewer for their
> > projects.
> >
> > --
> >
> > Tryggvi Björgvinsson
> >
> > Technical Lead, OpenSpending
> >
> > The Open Knowledge Foundation <http://okfn.org>
> >
> > /Empowering through Open Knowledge/
> >
> > http://okfn.org/ | @okfn <http://twitter.com/OKFN> | OKF on Facebook
> > <https://facebook.com/OKFNetwork> | Blog <http://blog.okfn.org/> |
> > Newsletter <http://okfn.org/about/newsletter>
> >
> >
> >
> > _______________________________________________
> > okfn-labs mailing list
> > okfn-labs at lists.okfn.org
> > http://lists.okfn.org/mailman/listinfo/okfn-labs
> > Unsubscribe: http://lists.okfn.org/mailman/options/okfn-labs
> >
>
>
> --
> Dan O'Huiginn
> Organised Crime and Corruption Reporting Project
>
> daniel at ohuiginn.net
> http://ohuiginn.net @danohu
> http://reportingproject.net
>
> _______________________________________________
> okfn-labs mailing list
> okfn-labs at lists.okfn.org
> http://lists.okfn.org/mailman/listinfo/okfn-labs
> Unsubscribe: http://lists.okfn.org/mailman/options/okfn-labs
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.okfn.org/pipermail/okfn-labs/attachments/20130605/fc4b730a/attachment-0002.html>