Edgewall Software

Changes between Initial Version and Version 1 of CodeReview/Wishlist


Ignore:
Timestamp:
Jun 18, 2011, 7:14:20 PM (8 years ago)
Author:
psuter <petsuter@…>
Comment:

Legend:

Unmodified
Added
Removed
Modified
  • CodeReview/Wishlist

    v1 v1  
     1[[PageOutline(2-3)]]
     2
     3= Code Review Wishlist =
     4
     5This page contains some wishful thinking about a hypothetical ideal code review system built into Trac (or a plugin).
     6
     7== What to review? ==
     8
     9=== Changesets ===
     10Low overhead. Only for ''post-commit review''. Review experimental repository clones / branches for ''pre-merge review''.
     11
     12=== Files ===
     13Or parts of files. High overhead. Good for auditing?
     14
     15=== Merge requests ===
     16Need to add the concept of a merge request to Trac first. But very useful and popular in modern DVCS systems.
     17
     18=== Patches ===
     19Nice for projects that already rely on submitted patches for their development model. High overhead for projects that are used to direct commit access.
     20
     21=== Screenshots ===
     22Optional bonus feature?
     23
     24== Configurable Workflow ==
     25
     26Code reviews should support configurable workflows (like tickets).
     27
     28=== Relevant states ===
     29* `new`
     30 * A new code review has been created
     31* `closed`
     32 * The code review has concluded. (Like for tickets, various resolutions may clarify what this means.)
     33* `assigned`, `reviewing`, `reviewed`?
     34 * Shows when a reviewer has been assigned, has started or finished reviewing the code.
     35 * This may be complicated by the need for multiple reviewers in one review.
     36 * Avoid over-complicating the process? (It's configurable. Some may need a more advanced workflow, some want to keep a low overhead.)
     37
     38=== Relevant resolutions ===
     39* `accepted` / `rejected`
     40 * For ''acceptance code review'' that results in a vote on inclusion.
     41* `completed`
     42 * For ''educational code review'' to increase knowledge transfer.
     43* `invalid` (`ignore`, `irrelevant`, `retracted`, `superseded`, `outdated`, `noneed`, `nntr` (''No Need To Review'')))
     44 * For various cases where a code review is aborted.
     45
     46=== Assigning reviewers ===
     47A 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.
     48
     49=== Voting ===
     50In 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.
     51
     52=== Veto ===
     53Some / all users may have a ''veto'' permission to overrule other votes instantly?
     54
     55== Commenting ==
     56Reviewers 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.
     57
     58== VCS integration ==
     59* Should a new changeset automatically create a new review?
     60* Should a special changeset commit message keyword trigger a new review? (and assign reviewers?)
     61 * similar to / extending CommitTicketUpdater?
     62* Should the existing changeset / diff / repository view pages be extended or modified?
     63* If files are reviewable, the browser could show the ''Review status'' of a file.
     64 * `Unreviewed` link to the form to start a new review.
     65 * `Reviewed` links to the review.
     66 * What if only an old revision was reviewed?
     67 * For directories show the percentage of reviewed files?
     68
     69== Ticket integration ==
     70* Should a code review simply be a ticket of ''ticket type'' `codereview`?
     71 * We get some discussion, notification and workflow support for free.
     72 * For reviewing patches / merge requests submitted as proposed fixes for the ticket's issue this seems ideal.
     73* Should a reviewer open new tickets for each complaint?
     74* Should a rejected post-commit code review result in one new ticket?
     75
     76== New navigation tab ==
     77If 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.
     78
     79One should see (be able to query?) ones submitted and assigned reviews and their most important properties. (Did I or someone else comment or reply?)
     80
     81Should reviews be  prepared and performed in a private ''staging'' area and only published when ready?
     82
     83== Other Trac integration ==
     84The code review system should be "natively" integrated in Trac in the sense that it works together with the appropriate core systems:
     85* Notification
     86 * People need to be notified when they are assigned as a reviewer or the status of their code review changes.
     87* Timeline
     88 * The timeline should show the important review events.
     89* Search
     90 * Reviews and review comments should be searchable.
     91* Wiki
     92 * Are there any appropriate special macros, link syntax, etc.?
     93* Preferences, Admin, Permissions
     94 * Configuration of code review permissions, admin settings and user preferences should be provided in the standard Trac way.