1. 18
  1.  

  2. 8

    I remember being completely blown away by Jane Street’s code review tool “Iron,” which was simultaneously much simpler and much nicer to use than any review tool I’d encountered before it.

    I went through this list to see how it stacked up – it can’t do everything, but it makes a pretty good showing. Here’s what it’s missing:

    Ability to view diffs in a work-in-progress state before sending them out for review.

    Iron does this, but in a stronger way: before you can send something out for someone else to review, you have to review (and “approve”) the code yourself.

    Ability to comment on a specific substring of a line.

    Have to simulate this with English words.

    Reviewers can signal the degree of their confidence in the code (e.g. “rubber stamp” < “looks good to me” < “full approval”)

    Iron only understood “we should merge this to parent” or “we should not merge this to parent,” which might reflect different degrees of confidence depending on the parent branch. (Of course this confidence could be expressed in high resolution to other people, but I’m not sure what tooling support would look like.)

    Automated “Round robin” reviewer assignment within a team.

    Not supported out of the box, but there were teams that scripted this.

    Code review history (including review comments) is saved somewhere, preferably indefinitely.

    Iron saves all comments forever, but not in a very easy-to-rediscover way. When I left people were working on a feature to make this more easily searchable/browseable. So it did do this, but the UX was bad enough that it basically didn’t. Probably the biggest missing feature; hopefully fixed now.

    Ability to rollback / revert a submitted diff from within the tool.

    Had to be done manually, and it was a bit of a hassle.

    Opt-in setting to auto-merge the PR once it’s been sufficiently approved.

    This would have been useful, especially for things like config changes.

    Ability to customize when a given CI workflow is run (e.g. upon initial review request, upon each update, upon submission).

    Couldn’t do this as far as I know. Everything always run on every (debounced) push.

    Customizable per-diff presubmits. Example: “Prevent this submission of this diff until PR #123 is deployed in prod”)

    Iron has the concept of “locks,” where you say things like “you can’t release this until X.” But “X” is a string of English text understood by humans, not something the tool was aware of. So a human could remove the lock erroneously.

    There was another interesting feature where you could tag certain commits as “introducing a bug” and other commits as “fixing a bug” and safety guards in place to prevent you from deploying software known to contain bugs but not contain their fixes, which sort of overlaps with this idea.

    I think that’s it? I think it ticks every other box. Well, except…

    For the purposes of this post, “code review tool” refers to a web UI for reviewing code

    So it wouldn’t count as a code review tool by this definition… but I’m alright with that. :)

    I gave a talk a few years ago describing how it works, if anyone is curious to see a slightly different approach. https://www.youtube.com/watch?v=MUqvXHEjmus

    1. 2

      Iron was never publicly available, was it? I remember seeing talks and videos, but I don’t remember actually seeing elisp. Might’ve happened after I quit using Emacs, though.

      1. 2

        No; Iron was never really released to the public. For some reason Jane Street used to publish the source code: https://github.com/janestreet/iron

        But as far as I know it was never actually buildable externally, because it had some in-house dependencies (like… the build system) that were not open sourced. Also the Emacs client was a separate project, and as far as I know was never published at all.

      2. 2

        Ability to rollback / revert a submitted diff from within the tool.

        Had to be done manually, and it was a bit of a hassle.

        This is built!

        Code review history (including review comments) is saved somewhere, preferably indefinitely.

        Probably the biggest missing feature; hopefully fixed now.

        Oh, honey

      3. 6

        I was surprised not to see any discussion about reviewing the description/commit message.

        This is a feature that I really miss in GitLab and GitHub as the merge request description is behind an issue number after merging occurs and those tools allow you to merge stacks of commits at a time which makes the problem even worse.

        Google’s Critique and Gerrit handle this fairly well. The description of the merge request is the description of the merge request so it is upfront and center. However there is still no way to explicitly review the commit message, just top-level comments that reference it with English.

        The only review system that I am aware of that handles this well is emailing patches where it is as simple to comment on the description as it is to comment on any file in the diff.

        1. 3

          That’s a great point – I updated the post with a few things in this vein.

          I also miss not having PR description match commit message in Github/GitLab. While I initially found it strange, I’ve become a fan of Gerrit’s handling of this – it allows the commit message itself to be reviewed (and commented/suggested upon), which is handy.

          1. 1

            If the PR only has one commit GitHub pre-fills the PR Title and Body with the commit message. It is a step in Gerrit’s direction at least.

          2. 1

            I yearn for the ability to review commit messages in GitHub. I love that about Gerrit. It really enabled enforcement of the idea that a commit message should be meaningful and tell a story about the patch.

          3. 2

            One that ends up being a convention if not built-in is the priority or confidence of a suggestion. Gerrit has this with its Range(-2, 2) rating system where the extremes allow full blocking or full passing while a 1 is more of a suggestion and 0 is just a comment weighing in.

            My team took this a wider with inline comments in GitHub reviews with things like :warning: being a -1, :stop: being a -2, :point_right: being a mild suggestion like a +1, :mortar_board: being a “today I learned” comment not requiring response or action, etc. I really liked this system a lot but some members of the team were better than others at adhering to it. I’ve not tried to get my current team to do it. I had a userscript that helped me remember!

            1. 2

              A surprisingly minor “feature” that really doesn’t add value but I still highly appreciate is good integration to add memes oder gifs. It’s completely unnecessary, but really adds a bit of fun and lightheartedness to the review process that I much appreciate on a long day of work.

              On a more serious note: A reminder and ranking system for incoming reviews is something i highly appreciate, particularly for large companies and teams where there is a lot of code review.