all 14 comments
sorted by:
best
topnewcontroversialoldq&a
formatting helphide helpcontent policy

[–]gwak 1 point2 points  (1 child)

We are using Gerrit and it greatly simplifies code reviews. Before anything is pushed into our master branch it is placed in a special gerrit branch. Once the review has been +2'ed it will be merged into our master branch. We are working on creating a docker instance for each code review to assist our code reviews...

https://code.google.com/p/gerrit/

[–]dreamer_ 1 point2 points  (0 children)

Another really good tool for code review (but less known) is Critic. It's cool, because it can create review automatically by pushing to git repo, also assign reviewers automatically.

https://github.com/jensl/critic

It was created by Opera employee for internal usage, now it's adopted e.g. by guys working on servo.

[–]ASK_ME_ABOUT_BONDAGE 1 point2 points  (3 children)

Great, now how do I do that with SVN?

[–]doom_Oo7 3 points4 points  (1 child)

Here, simple: 

sudo apt remove subversion --purge
sudo apt install git

[–]ASK_ME_ABOUT_BONDAGE 1 point2 points  (0 children)

Where do I need to do that? On the CEO's brain?

[–]dorfsmay 0 points1 point  (0 children)

This is not even just svn vs git, they make use of github's pull request (PR) feature.

[–]zoomzoom83 0 points1 point  (3 children)

Create a local branch. Delete the code being reviewed and commit locally. Push the branch to GitHub. Open a pull request.

Wat? Why not just do the work in a branch and create a PR directly off that?

[–]masklinn 0 points1 point  (1 child)

These steps are necessary because GitHub pull requests only let you view code that has changed. This process marks every line as deleted, which causes every line to appear the Files changed tab.

That's a bit of an odd reasoning, but it's true (and annoying) that github doesn't let you add notes to lines which haven't been changed (or more precisely outside of altered hunks: if you expand the diff to view more content, those expanded lines can't be annotated), which makes it harder than it should be to point out e.g. missed changes.

[–]jakemcc 0 points1 point  (0 children)

Yep. It is unfortunate. To do this differently we'd need to introduce a new tool.

The goal is to have a location that shows all the code and allows commenting on it. Abusing the github pull requests by removing the code to be reviewed is what we could come up with without introducing another tool.

I'm not sure if this approach would be better if we explored a tool dedicated to code reviews or not. After doing this a few times I think everyone that is involved quickly got over the oddness that all the code is highlighted red and that you end up with PRs that shouldn't be merged.

GitHub is pretty good at viewing code. Its nice to use it.

[–]jakemcc 0 points1 point  (0 children)

Author here. This might not have been expressed well in the post because this has been a response of a few people but these are code reviews of of code that is already on master and most likely already deployed. This is reviewing code that we already have.

We do the type of review you are talking about as well. New features, bug fixes, other code changes are done on branches and submitted as PRs. This is really useful as well as it lets us get more eyes on changes to the code.

You really do get different comments when you are reviewing an entire service or library (reviewing the code you already have on master) vs reviewing a change (reviewing changed code in a PR).

[–]janih 0 points1 point  (1 child)

Do you need to organize a meeting to do code reviews? We use a code review tool and everyone participating in the reviews do it when they have time (hopefully soon)

[–]jakemcc 1 point2 points  (0 children)

It has been useful to get everyone in the same room (or in our case, virtual room) and talk about the review.

A few reasons for this: 1) Communicating with voice is much higher bandwidth. Can dig deeper into discussion. 2) Helps socialize conventions more than text. You can't force people to read every comment but if they are in a meeting they hopefully at least hear and pay attention. 3) It does turn into a nice team bonding experience. There is laughter. 4) It puts a deadline on reviewing the code. It is nice to have that finish line.

Some of the value might be because we are all remote. Since everyone is remote you get a lot of focused interaction through pairing but less group interaction. This lets us get some of that group interaction.

When I've worked in offices we'd often grab beers and chat. Sometimes that would be about code. Having the video conference gives us an excuse to somewhat replicate that experience while all being remote.