Code Review Wishlist

This page contains some wishful thinking about a hypothetical ideal code review system built into Trac (or a plugin).

What to review?

Changesets

Low overhead. Only for post-commit review. Review experimental repository clones / branches for pre-merge review.

Files

Or parts of files. High overhead. Good for auditing?

Merge requests

Need to add the concept of a merge request to Trac first. But very useful and popular in modern DVCS systems.

Patches

Nice for projects that already rely on submitted patches for their development model. High overhead for projects that are used to direct commit access.

Screenshots

Optional bonus feature?

Wiki pages

Producing high-quality pages also imply some kind of review system. One can always add review comments "inline", but this can quickly clutter the page and makes reading more difficult, so a lot of Wiki systems have the notion of an associated "Talk" page. While a talk page has the benefit of leaving the content focused on its main matter, it also have the disadvantage of being too "disconnected" and the discussion there can quickly become out of sync with the content page.

It would really be nice to be able to see the pristine page content, but also access an alternate view with superposed review annotations. Likewise for editing, one should be able to edit individually:

• the content alone
• the review layer alone, for example when creating a review
• the content and review layers at once, for applying the changes requested by a review and simultaneously update the review comments

I'm not sure at this point how well this idea fits with the rest of the code review topic - so for now just take that as a related topic ;-)

Configurable Workflow

Code reviews should support configurable workflows (like tickets).

Relevant states

• new
  • A new code review has been created
• closed
  • The code review has concluded. (Like for tickets, various resolutions may clarify what this means.)
• assigned, reviewing, reviewed?
  • Shows when a reviewer has been assigned, has started or finished reviewing the code.
  • This may be complicated by the need for multiple reviewers in one review.
  • Avoid over-complicating the process? (It's configurable. Some may need a more advanced workflow, some want to keep a low overhead.)

Relevant resolutions

• accepted / rejected
  • For acceptance code review that results in a vote on inclusion.
• completed
  • For educational code review to increase knowledge transfer.
• invalid (ignore, irrelevant, retracted, superseded, outdated, noneed, nntr (No Need To Review)))
  • For various cases where a code review is aborted.

Assigning reviewers

A new code review should be assignable to specific reviewers (Trac users) or to a group of reviewers (permission groups?). Depending on the workflow, one, some or all of them should have to perform some action on the review.

Voting

In some workflows, the individual reviewers are required / allowed to vote on a code review. Voting could be a state transition. A special transition operation could tally and check the votes against a configured threshold. Depending on the outcome it remain in the same state (and wait for further votes) or resolve the review to the accepted or rejected states.

Veto

Some / all users may have a veto permission to overrule other votes instantly?

Commenting

Reviewers and submitter need to discuss the reviewed code. Referring to specific section or lines of code should be easy, by attaching and showing comments and replies inline / side-by-side with the code.

VCS integration

• Should a new changeset automatically create a new review?
• Should a special changeset commit message keyword trigger a new review? (and assign reviewers?)
  • similar to / extending CommitTicketUpdater?
• Should the existing changeset / diff / repository view pages be extended or modified?
• If files are reviewable, the browser could show the Review status of a file.
  • Unreviewed link to the form to start a new review.
  • Reviewed links to the review.
  • What if only an old revision was reviewed?
  • For directories show the percentage of reviewed files?

Ticket integration

• Should a code review simply be a ticket of ticket type codereview?
  • We get some discussion, notification and workflow support for free.
  • For reviewing patches / merge requests submitted as proposed fixes for the ticket's issue this seems ideal.
• Should a reviewer open new tickets for each complaint?
• Should a rejected post-commit code review result in one new ticket?

New navigation tab

If not all code review features are integrated into existing pages, a new navigation tab is needed where code reviews can be created and viewed etc.

One should see (be able to query?) ones submitted and assigned reviews and their most important properties. (Did I or someone else comment or reply?)

Should reviews be prepared and performed in a private staging area and only published when ready?

Other Trac integration

The code review system should be "natively" integrated in Trac in the sense that it works together with the appropriate core systems:

• Notification
  • People need to be notified when they are assigned as a reviewer or the status of their code review changes.
• Timeline
  • The timeline should show the important review events.
• Search
  • Reviews and review comments should be searchable.
• Wiki
  • Are there any appropriate special macros, link syntax, etc.?
• Preferences, Admin, Permissions
  • Configuration of code review permissions, admin settings and user preferences should be provided in the standard Trac way.