Code Review: improvements to better support iterative review

[David Yang]

Hey guys

Plastic SCM kicks Git's butt in pretty much every way except when it comes to code reviews.

Below are some features which are commonly available in the Git world (by using pull requests in most Git hosting services), which make iterative code reviews easier and would be most appreciated in Plastic SCM. I think the Branch Changes Window probably achieves 80% of this already (if you embedded that instead of a simple Diff Window for branch-based code reviews).

Here is an example of what pull request overview page looks like in Azure DevOps (which shows a number of the features I talk about below):

 

These are only really needed for code reviews for branches (not code reviews for changesets):

1. Timeline of comments and updates: As you can see above, it shows a simple timeline of when the code review was created, code review comments (with a snippet of the relevant code), and subsequent updates (update = push, which might contain several commits, but for Plastic SCM each checkin should probably be it's own update). Replies should probably be grouped under the original comment, rather than have its own entry on the timeline.
2. Easily compare updates to see if comment is resolved: If I click on a comment, it should take me to a diff of the latest changeset on the branch VS the changeset on which the comment was made. This lets reviewers easily see whether the comment was addressed and how.
3. (Optional) Show comments in-line with code: This one might be the trickiest to get working with the diff tool, and isn't necessary for the other improvements to work. What it does do is help make unresolved comments a lot easier to see. Here is what this and #2 look like in Azure DevOps:
4. Ability to mark comments as resolved: Original comments (not replies) should have a property to indicate its status (eg, active, resolved, can't be fixed, etc). If a comment is resolved or otherwise closed, it should be collapsed by default (eg, in the timeline, in-line comments, bottom comment pane, etc) so it is easy to see just the unresolved comments. Here is the same screen but with the comment collapsed (default when comment is resolved). Comment can be expanded by clicking on the blue circle:
5. Unresolved comments should carry over to updates: For the above to work properly, unresolved comments should be visible on new changesets for that branch, so they can't be missed. Currently, they will only appear in the bottom pane, and clicking on it changes the right-side diff window to the original changeset (which is not what we want). I understand there will be some complexity as comments are stored by line number (which could change as a result of code additions/deletions/moves), but hopefully this is not insurmountable with the really awesome xdiff tools you already have?

Would also be grateful for an update on how development on the Code Review system is going overall (there are a number of UserVoice suggestions where you've mentioned an overhaul of the system is in the works, but those suggestions date back several years and there doesn't seem to be an update)?

Hopefully you guys are still committed to improving the Code Review system. As I said, Plastic SCM pretty much kicks ass in every way - this is one of the only things I really miss from the Git world. Hopefully with your existing tools (eg, Branch Changes Window and Xdiff), the above features won't be insurmountable?

Kind regards

David

 

Edit: Here is a link to this item on UserVoice. Please upvote if you agree.

[psantosl]

Hi David,

 

First, thank you for taking the time to compile all this great feedback.

 

I'm the CTO here at Plastic, so let me share with you some of the designs we have for the upcoming Code Review system. Our plan was to implement it last year, but we got swamped by a ton of other smaller but still important things.

 

First let me share what we consider the keys of the new code review system:

I'm copy/pasting from internal docu :-)

 

And this is how the overall design of the review window and its elements look like.

 

 

The key elements for Code Review are:

• Comments are now together with the diff (on a side panel).
• Comments have replies, so you can handle threads there.
• The key is that you can "ask questions" and "ask for changes".
• And then it is very easy to track if those questions were answered or if the actual changes where made, which is one of the things you wanted to see.

How?

You'll type a special comment with a mark saying it is a change request.

 

Then it will be very easy to track if the change or question was answered, and double clicking will show you the right diff:

 

When you checkin, you'll be able to select (or using a comment) the "change" you are addressing:

 

 

Finally, we'll have a conversation tab, to discuss general things outside the diffs themselves:

 

 

And you can review changeset by changeset too, not only entire branches, which is great IMO:

 

The last thing will be "review scripts":

 

 

 

And that's just the beginning.

 

So, in summary: we have put together a bunch of ideas thanks to all the feedback we got and our own practice of code reviews. Now it is a matter of finding the time to implement this in an incremental way.

 

We want to finish some key changes on the server, which will enable the next Cloud version (and also key for some huge customers we have with 3000+ seats). And we also have to improve the single branch workflow for developers on game teams... so it is not yet decided when Reviews are going to happen.

 

pablo

[David Yang]

Thanks Pablo, that's awesome to hear!!

Sounds like what you guys have planned covers everything in my post and more (great idea being able to link a changeset to a specific "change" request). The big combination window is a tad overwhelming at first (maybe move the bottom pane to the conversation tab, and simply have a pop-up warning if trying to close the code review or merge the branch with unresolved questions/changes)? Otherwise, everything looks great!

Absolutely understand you need to prioritise the key changes needed to support your big customers. Would be really great to see these Code Review improvements after that though! Currently this (and perhaps lack of built-in issue tracker - I'm having some difficulty with the MantisBT integration, which I've posted about separately) are the only big things I'm really missing about the setup I had with Git + Azure DevOps. Otherwise, Plastic is absolutely amazing - the underlying branch mechanics, branch explorer and diff/merge tools are miles and miles ahead of anything else!

David

 

PS: By the way, what is the issue people are currently having with single branch workflows in Plastic?

I currently use what is essentially a single branch workflow with Git (but that is mostly due to Git's limitations), so let me know if there's any way I can help. Essentially, I create temporary task branches, "squash merge" onto master when finished (with Azure DevOps automatically deleting the task branch, but preserving a history of the task branch's commits in the completed pull request).¹

If people want it, I think this would be fairly simple to implement in Plastic. With "squash merging", you would just need to enforce the deletion of the task branch to prevent people reusing it and creating evil twin conflicts. And preserving the changeset history of the "deleted" task branch could be done by "hiding" the task branch rather than actually deleting it (I think I read somewhere that that's basically what Azure DevOps, Github, etc do with their pull requests - they just move the task branch to a branch called pull/<name> and somehow hide branches with the 'pull/' prefix from Git).

Edit: Actually you could already do this with attributes and filters.

_____

¹ This seems to be Microsoft's own approach. I'm guessing they also had trouble with other Git workflows - the much-hyped GitFlow model is actually really hard to use with Git in practice, firstly because commits don't "belong" to a branch in Git (although I know you guys are working on something which makes this a lot better - I'll drop you a separate line on that 🙂), and secondly because Git doesn't handle merging well when there are cherry-picked commits (Git thinks they are different commits which change the same lines, so it reports a merge conflict despite those lines being changed in the same way). Piece of cake with Plastic though 👍