About code review

[Pierre-Yves Chibon]

Good Morning everyone,

Ralph and I have tried to move our team to a pull-request based development
practice. The idea is that every piece of code of every application that is
released should go through a pull-request/code review. That means application
that are being dev, do not necessarily need to go through this process.

The code review can be as simple as checking that there is nothing standing out
in the code while reading it, asking for a comment in the code if there is
something a little hard to understand. It can also be as complex as pulling the
changes locally and run them to see if they behave as expected.
Most often I have done the former though :)

Code review does not mean you are formally acknowledging that there are no bugs
in this piece of code. More that you have been through it, understand it and
that it seems to make sense to you.
The person who wrote the code is expected to have tested it, the reviewer is
more tasked to check if the code make sense, if there are optimization that
could be done and using this process, we can improve the understanding of our
apps in the team, meaning that if there is a fire to pull out, the code will
look more familiar to more people.

All this to say that, everyone can do code review, I've missed more than my
share of typos and small bugs in reviews, but I've also found some small issues,
asked for some clarifications and so on.

Code review is also a good place to fight technical debt. Quite often we can do
something that works and the reviewer points out that this is not the best way,
which motivate us on fixing things (that's at least my experience).

Asking for code-review isn't necessarily nice, it gives the feeling you're
asking someone to do work for you.
So Ralph and I put in place a few tools allowing to find the opened
pull-requests so that people wanted to do some can check them, once or twice a
day or more, see if there is anything new and act on it without the need for the
developer to ask.

So pull-requests can be found:
* on #fedora-apps, either by the github or fedmsg bots
* on http://ambre.pingoured.fr/fedora-infra/ for all the opened PRs on
  github.com/fedora-infra
* available on irc using the .pulls command, for example:
    .pulls pagure <- the PR of the project tagged as 'pagure' on pagure.io or
                     part of a 'pagure' group on github
    .pulls fedora-infra <- the PR of all the projects tagged 'fedora-infra' on
                           pagure or part of the fedora-infra group on github

I personally pinned the URL in the second bullet point above in my firefox and
check it a few times a day.
If people are interested, I could see about adding support for pagure in there
and other platforms if we want/need.

I hope this will motivate everyone to regularly check for new pull-requests and
take a few minutes to go through them and see if they can spot problems.


Thanks,
Pierre