Should we consider Review Ninja for Code Review?

r0bby · May 21, 2016, 5:24am

So there’s a a FOSS project made by SAP called Review Ninja – it is totally awesome. They shared the booth with us at OSCON.

You comment on the PR in Review Ninja and it is stored as a comment on the PR iitself in GitHub.

If you wanna to, maybe try it out when you review PRs. I plan to use it this summer and will write up . my experience. We can even deploy an instance ourself – but I’d prefer we not do that personally – plus we have nowhere to put it.

pascal · May 23, 2016, 11:42am

What are the main benefits it gives you over just doing the review on the GitHub PR?

r0bby · May 23, 2016, 7:22pm

This page has all your questions answered. It’s just sleek. I’m gonna use this summer…so will report back – likely in the form of a blog post.

burke · May 23, 2016, 7:35pm

Their answer to “Why should I use ReviewNinja”:

ReviewNinja gives code review more structure than Pull Requests alone offer. It also provides a status overview so that team members in other departments, such as product management or design, can quickly grasp the state of the project.

It appears to add a voting system to PRs and help with workflow. The problem being solved isn’t clear to me either, @pascal.

We’ll look forward to your blog post, @r0bby!

-Burke

michael · May 23, 2016, 7:51pm

I haven’t yet read up on Review Ninja, but generally the consensus last week at OSCON from major/large open source projects like OpenMRS is that using pull requests alone for code review are insufficient for various reasons. Here’s a good argument for Gerrit, another player in this space, that also came up last week:

gist.github.com

https://gist.github.com/mbbx6spp/70fd2d6bf113b87c2719

README.md

# Gerrit vs Github: for code review and codebase management

Sure, Github wins on the UI. Hands down. But, despite my initial annoyance with Gerrit when I first started using it almost a year ago, I am now a convert. Fully. Let me tell you why.

Note: This is an opinionated (on purpose) piece. I assume your preferences are like mine on certain ideas, such as:

* Fast-forward submits to the target branch are better than allowing merge commits to the target branch. The reason I personally prefer this is that, even if a non-conflicting merge to the target branch is possible, the fact that the review/pull request is not up to date with the latest on the target branch means feature branch test suite runs in the CI pipeline reporting on the review/PR may not be accurate. Another minor point is that forced merge commits are annoying as fuck (opinion) and clutter up Git log histories unnecessarily and I prefer clean histories.
* Atomic/related changes all in one commit is something worth striving for. Having your developers think about the granularity of their commits in terms of relatedness, cohesiveness, and deployment units makes the changes more manageable. This is my claim and the only evidence I have of it is the difference I have witnessed on teams that went from 1..N commits per feature branch to 1 commit per feature branch. The difference I saw was an awareness one. Developers started to think about viability of deployment of that one unit of change to each of the environments. That wasn't the case before we made the change to one commit per feature branch policy. You can enforce this using Github even, but it's not provided by the Github interface.
* Being able to publish a review in draft form without prefixing with "WIP" or other general convention (which is totally possible and scriptable in Github; I know I have done it previously) simply makes querying and permission/visibility of those reviews/PRs easier to manage and filter out/in when necessary. Perhaps a minor point.
* Limiting permissions can provide some merits to helping manage releases and codebase repositories more consistently via automated builds/CI/CD setups.

This file has been truncated. show original

r0bby · May 23, 2016, 9:03pm

I actually merge by hand, not using github’s pull request ui…I typically manually squash/cherry-pick…sometimes I rebase. Cherry-picking allows a linear history.