Upcoming: code reviews in the age of pull requests
We are working on a new code review system. Yes, I know this is the kind of blogpost that will quickly get out of date, but I felt the urge to share with you, dear Plastic user, what we are working on.
I'm going to share what we are doing, what you can expect, how it is going to be better than any pull request system to date, why it took us so long to get started, and how code reviews are central to our version control vision.
Just to give you an overview of what it will look like, let me share a screenshot of one of the design docs:
The story – how it all got started
Rewind back 10 years. We got some requests from users saying we should build our own code review. I thought it was wrong. It was the days of big code review systems, in fact, they enabled proper "code inspections" with a scribe, presenter and everything. SmartBear was around with an incredibly powerful code review system. Doing code reviews was not our business. Merges was.
But we ended up saying yes and we built the simplest possible code review system. It just allowed you to add a few comments to diffs and track the status of the review. Surprisingly, many teams started to use it.
The world was changing. I mean, pull requests are now the change packages of the vast majority of developers in the world, and they are "just" a very simple code review system. Everyone now expects the version control to provide code reviews thanks to GitHub, then GitLab, Bitbucket and the like.
Historically, we tried to remain "process agnostic". But we radically changed our view about this a few years ago. I mean, should we provide a code review system? As I already said, this is now something users expect. Somehow, the responsibility switched to the version control providers. A world with simpler and more effective tools.
Thus, it was time to fix the top priority request in our UserVoice: add reply to code reviews.
Our vision for code reviews
I'm sure you heard of this:
"Programs must be written for people to read, and only incidentally for machines to execute"
You must write code thinking about the fellow programmer, not the machine. We all somehow accept this these days, don't we?
What if we checkin with reviewers in mind? What if every single checkin tells part of the story? What if every checkin comment really talks to the person who is going to review it, and helps them focus on what really matter and helps them save precious time?
That is our mission, actually: create tools to make teams more efficient by letting them focus on what really matters. That's why our diff is semantic, that's why it is side by side, and that's why we provide a Branch Explorer.
And we always believed in this. For example, I wrote a full chapter in our Plastic Book about why reviewing each task is great .
But, somehow, we left the code review as an exercise to the reader. It is like, "yes, you should review each task branch, but you're on your own to do it".
That's what we want to fix. Strong code review is going to be part of Plastic. Built-in.
Pull requests are broken
"Everyone spots 3 issues on a 3-files pull request. But one with 150 files gets immediately approved". Ever heard stories like that?
Another one, this time from Atlassian:
So, we have a great opportunity to get them fixed.
- What if you can skip hundreds of simple "renames" or irrelevant noisy changes? We can do that with the help of our semantic diff technology. We also have ideas to help the author help the reviewer group those changes.
- Changeset by changeset reviews with good story-telling and explanations to understand even the toughest refactors.
- And new ideas like "review scripts" where the author explains the changes.
Well, that's the goal: improve code reviews, make them much better. Let's see if we can do it.
Key principles for the new system
When we started designing the new code review system, we wanted to achieve the following goals (again, let me share a picture extracted from the design doc):
Now, I'll share how we plan to achieve this.
Layout
Code reviews are going to have different layouts depending on the situation. When you start reviewing the code, you'll go file per file, checking diffs and adding comments.
Once the author goes back to check comments, questions and change requests (more on this later), they'll see a different layout. Going file per file won't be interesting anymore, it will be all about checking what the reviewer asked to change and the questions to answer.
Comments don't go inline
We don't like review comments inline with the code. They make the code more difficult to read, and you lose sight of what the code is supposed to be doing.
This is what we plan to display:
Right now, in the current review system, the comments are piled at the bottom which is *worse* than inline comments. So yes, we expect to do way much better with the new version.
Threaded conversations
This is something I always wanted to have. I really like to branch a conversation to address a specific thing. Like you say something, Vio replies, but I want to answer *your* comment, not Vio's.
This is what we want to achieve:
Changes and questions
Changes are questions that are the secret sauce of the new code review system. I think if we manage to implement them successfully, they'll really change how code reviews are done.
They structure the conversation between authors and reviewers and let them keep focused on what really matters at each moment.
This is better seen with an example: imagine you want to ask me to change something. Instead of just a comment, you enter a "change":
Then, when I open the code review again, I see the list of changes:
Just by clicking the change, I jump to the right file and see the diff and your comment.
Then, I can make the change and specify that I addressed your change:
Of course, it will also work entering a specific comment in the checkin, but GUI support will be good too.
Now, the next time you'll enter the review, you'll see this:
And it will be obvious to you to see how I fixed your request (just click and go to the diff of my code), and also how I discarded the other one by just entering a comment (and you'll check whether discarding it makes sense or not).
Also, as you can see, if you mark comments as "questions", they'll show up in a list and it will be trivial to see if they were answered or not.
Changes and questions are the feature that I'm more excited about. They really help you do reviews in a different way and achieve better results.
And this is how the diff will be annotated:
Review comments (R)
Changes (C)
Questions (Q)
For example:
Comment changes to art
As you know, game teams are important to us, so the review system won't be just about code but also about images.
Conversations
Sometimes you need to have a global conversation, not related specifically to a given change, but more about the task in general.
This is when conversations enter the scene.
This is nothing really new since pull requests already implement this. So unlike with "changes and questions", there won't be a huge innovation, but this is something users wanted badly.
Multiple reviewers, multiple developers and guests
So far, a review only had a single reviewer. This no longer matches reality. Users asked us to have:
- Multiple reviewers: So a review can only be approved if all of them mark it as reviewed.
- Multiple authors: So they are all notified when the review is reopened or changes status.
- Guests: they can review, but it is not mandatory, but they are in the loop.
Review scripts
This is how we'll implement the "checkin with reviewers in mind" part. Our goal is to enable an asynchronous conversation between authors and reviewers, through checkin comments and review scripts.
Consider a change like this:
And suppose you entered the following checkin comment:
[file: Simple.cs] Contains the redesign of the calculation. [file: Calc.cs] Contains the code extracted from BaseCommandsImpl. [group: Just method renamed: Bar.cs, Foo.cs, BaseCommandsImpl.cs]
And thanks to this comment, the reviewer will see the diff as follows:
You are actively helping the reviewer! You are helping them focus on Simple.cs first because this is the most important change, then go to Calc.cs and you clearly say the next group of files have changes that are not that important.
This is the feature that I like the most! :-)
There will also be "scripts" to focus on a given diff and help the reviewer even more. Consider this:
This is a refactor of the Calc() method. Check first [diff: cpp.c : 2]
Which means when you click [diff: cpp.c:2] the second diff in the file will be opened.
Now check the following diff:
And suppose you enter a comment like this:
[refactor: Check how I extracted the method and changed order: BranchExplorerLegendItems.cs : left:167-174@cset:170, right:180-187]
That would help the reviewer exactly diff the moved code. Wouldn't it be awesome?
Review scripts will be easily entered as checkin comments, but we also plan to let you enter them later, during review creation:
Availability and next steps
An early prototype is already available to use with release BL3388 and we'll be gradually deploying improvements. The goal is to have the new threaded conversations real soon on Windows, then changes and questions, then release on macOS, Linux and later start to add comments on images, conversations and review scripts.
Feedback welcome
Please contact us with feedback and let us know if you are interested to join our internal Basecamp to have a direct line to developers.
Enjoy!
0 comentarios: