So how about this - instead of a Defect, or a Blocker, how about an
In your workflow, when a developer successfully argues their case,
what happens to the Blocker? ie., what's the verb? Is it
"overridden"? "Removed"? "Released"? "Closed"?
Issue could work, and I believe some other code review software uses
this term too. However, now we reach your original problem. "Issue"
will sound to users like something that integrates with their issue
Christian Hammond - chi...@chipx86.com
Review Board - http://www.reviewboard.org
VMware, Inc. - http://www.vmware.com
I would like to provide some feedback, on the whole concept.
From your mock-up and description it seems like you are trying to mark
a difference between something that should be fixed before commit and
a general comment on that bit of code.
Whilst a tick box that marks the comment is great (and simple which is
always a bonus) - my concern is that its too simple.
I wrote a code review tool for my last company that was built on top
of Telelogic Synergy.
When developers were reviewing code they wanted to make a distinction
between comments and issues/bugs/problems etc.
However we found that a simple distinction of comment / bug was too simplistic.
What we did what have a configurable list of issues which also
contained a comment / fix next time
In your mock up you could replace the tick box with a drop down box
that defaults to comment / general observation.
But if the user clicks on the drop down box a configurable list
appears. For us the list contained things like
Logic, Standards, Performance, Defensive etc.
This way the table of issues can be more informative.
Also much as statistics and historical data gathering are not the
first love of developers - it is good to have the information
available and to be able to spot a trend - maybe people regularly are
ignoring coding standards etc.
I would love to see something like this - I think it is still simple
and uses little more real estate on the UI. But is allows each user
of ReviewBoard to further tailor the tool for their own environment.
Hope this is helpful and maybe some useful feedback on what I would like to see.
Cheers for the great work
In our world, we've always used the simple terms "Ack" and "Nack" to
express the high-level results of a code reviews. (A Nack would then be
followed by an example of why it failed to pass muster).
I'd suggest the following workflow:
1) An engineer submits a code review
2) Reviewer reads the code and sets either the "Ship It" (aka Ack) flag,
or sets the "Nack" flag and provides code review comments
3) Assuming Nack flag, the original engineer then makes changes and
submits a new version of the patch.
* This action (replacing the original patch with a newer one) should
CLEAR the ShipIt or Nack flags if either are set.
* This will allow easy filtering for patches. There will essentially
be "Needs Review" (no flags set), "Needs Revision" (the "Nack" flag is
set), "Awaiting Submission" (the "ShipIt" flag is set) or "Submitted"
(The "Submitted" flag is set).
Of course, this also implies that the dashboard should be able to filter
based on these flags.
Also, my recommendation for terminology would be "Needs Revision". It's
fairly non-confrontational, but it gets the point across.\
><> Jan Koprowski
Yes. I Love Poland Summer too and :) I just back from Bory Tucholskie
:] I get wash in lake on the morning and drive to work straight from
the forest :) This is beautiful!
><> Jan Koprowski