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
On Thu, Apr 21, 2016 at 10:04:07AM +0200, Pierre-Yves Chibon wrote:
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
I've updated the script generating this page to include PRs from projects hosted on pagure tagged with the `fedora-infra` tag (cf the settings page of the project).
This shows that we have now 23 pull-requests in our queue, with at least 3 that do not have any comments on them.
Pierre
infrastructure@lists.fedoraproject.org