In my last post, I talked about an extension for Review Board that would allow users to register “defects”, “TODOs” or “problems” with code that’s up for review.
After chatting with the lead RB devs for a bit, we’ve decided to scrap the extension.
[audible gasp, booing, hissing]
Instead, we’re just going to put it in the core of Review Board.
[thundering applause]
Defects
Why is this useful? I’ve got a few reasons for you:
- It’ll be easier for reviewees to keep track of things left to fix, and similarly, it’ll be harder for reviewees to accidentally skip over fixing a defect that a reviewer has found
- My statistics extension will be able to calculate useful things like defect detection rate, and defect density
- Maybe it’s just me, but checking things off as “fixed” or “completed” is really satisfying
- Who knows, down the line, I might code up an extension that lets you turn finding/closing defects into a game
However, since we’re adding this to the core of Review Board, we have to keep it simple. One of Review Board’s biggest strengths is in its total lack of clutter. No bells. No whistles. Just the things you need to get the job done. Let the extensions bring the bells and whistles.
So that means creating a bare-bones defect-tracking mechanism and UI, and leaving it open for extension. Because who knows, maybe there are some people out there who want to customize what kind of defects they’re filing.
I’ve come up with a design that I think is pretty simple and clean. And it doesn’t rock the boat – if you’re not interested in filing defects, your Review Board experience stays the same.
Filing a Defect
I propose adding a simple checkbox to the comment dialog to indicate that this comment files a defect, like so:
No bells. No whistles. Just a simple little checkbox.
While I’m in there, I’ll try to toss in some hooks so that extension developers can add more fields – for example, the classification or the priority of the defect. By default, however, it’s just a bare-bones little checkbox.
So far, so good. You’ve filed a defect. Maybe this is how it’ll look like in the in-line comment viewer:
A defect has been reported!
Two Choices
A reviewer can file defects reports, and the reviewee is able to act on them.
Lets say I’m the reviewee. I’ve just gotten a review, and I’ve got my editor / IDE with my patch waiting in the background. I see a few defect reports have been filed. For the ones I completely agree with, I fix them in my editor, and then go back to Review Board and mark them as Fixed.
All fixed!
It’s also possible that I might not agree with one or more of the defect reports. In this case, I’ll reply to the comment to argue my case. I might also mark the defect report as Pass, which means, “I’ve seen it, but I think I’ll pass on that”.
I think I'll pass on that, thanks.
These comments and defect reports are also visible in the review request details page:
A defect has been filed.
All fixed up.
It's all good - just pass this defect report.
Thoughts?
What do you think? Am I on the right track? Am I missing a case? Does “pass” make sense? Will this be useful? I’d love to hear your thoughts.
You know what I’d also love to see? A list just of the defects, so you can check them off or see how many you have left to fix. I actually do something like this with comments – when I’m working through a bunch of comments, I’ll add a comment saying “fixed/OK or ignored” so that I know which ones I’ve followed up on. It would be really nice to be able to see how many you have left.
Another idea is that the reviewer could set “Ship It if no defects” so that their review status would be changed to a “Ship It!” automatically when all of the defects are fixed.
P.S. I love how the stuff that you work on, you can blog about and share with the world! 🙂
You know what would be really cool? Marking code as “WTF” and thus implementing a metric “WTFs per review”, as per http://www.osnews.com/images/comics/wtfm.jpg
@Tara:
Hey – those are both great ideas. Especially the second one – how many times have I said “Just fix this one thing, and it’s a Ship-It?”. Great!
The first one I’d already planned, but didn’t have the UI set up. I’m still wondering if its useful to display a single comment (marked “defect”) out of context from a thread. I imagine this wouldn’t be a problem is the defect report was the first comment of a thread, but if it was from a thread like this:
Reviewer: Neat piece of work here. I’m curious, why did you use foo instead of bar?
Reviewee: Well, I thought lorem ipsum blah blah.
Reviewer: Oh, well, that’s not entirely true. Blah blah lorem ipsum. *Defect report*.
And then in the list of defects, you’d only get that last comment, and you’d need to get into the thread to see what the hell they’re actually talking about.
It’s not a huge problem, but a significant one. Still figuring out how best to handle. Let me know if you think up a solution for me. 😀
Thanks for the suggestions!
@Nikolai:
I love it. 😉 That just might be the perfect easter egg for my statistics extension…
Thanks for posting, the both of you!
Best,
-Mike
Perhaps you could display the entire comment thread? That’s really the only way that a comment would make sense. How hard is that? Don’t you have the parent comment ID for a comment? I don’t know how the data structures are, but perhaps there is a way to mark the entire comment thread as a defect and then display it all?
Hope that makes sense and good luck!
@Tara:
Yeah, that was my first approach. The problem is with long threads. If we include the long thread (or let the “issue” expand to the long thread), then we’ve got the long thread duplicated twice on the same page.
What I might end up doing is just linking the issue to the point in the conversation where its reported. So if you’re looking at the issue list, and you’re confused about what exactly one particular issue is talking about, clicking it hops you down where it occurred in the conversation, which should help you figure it out.
That’s the theory, anyhow. :p
Thanks for posting,
-Mike
Pass-for-now vs. Pass-forever
The [Fixed] [Pass] formlet needs a couple of tweaks:
1. In the mockup, I couldn’t tell if there was room for a comment attached to the reviewee’s [Fixed|Pass] selection. Sometimes that 1-bit answer isn’t enough.
2. In particular, I think you might want to distinguish in the default UI between “I’m passing on this because I don’t think it’s important [never fix]” and “I’m passing because while this should be fixed, it’s not as important as fixing the original goal; I’m going to check it in as a new bug”.
I’m also just not very comfortable with the term “Pass”. In many contexts that means “This passes (quality check or whatever)”, pretty much the opposite of what’s meant here.
Need two succinct terms for those states. “I’m adding a bug” is easy: “Defer”. I don’t have a good one for “I don’t believe it’s worth fixing”. “Ignored” is too cavalier. “Not important”? “Insignificant”?
As a future hook, the “Defer” choice should have a way to shove a new bug report into your bug tracking database. “That schmuck Bela just deferred this comment `blahdity blah’ from the fix for PR 2468, thus creating a new bug…”
>Bela<
I had to buy a license for code collaborator because the main features that were missing to me in review board were defect tracking and reporting
was any of these features added lately or are planned to be added soon?
@Ari:
Hey, thanks for posting!
I’ve just put up a demo of the issue tracking feature I’m hoping to get merged. It’s still in the very early stages, and I’d love to get some feedback from you: http://mikeconley.ca/blog/2010/10/21/review-board-issue-tracking-a-sneak-peek/
With respect to issue tracking and reporting: you’re probably best to stick with Code Collaborator, at least for the time being. The reporting extension and issue tracking are still in the very early stages, and probably won’t get integrated for a few months.
Let me know if you have any questions. All the best,
-Mike