Couldn't agree more. We've tried to stay away from any sort of thick policy for a while. I feel one of Review Board's main strengths is that very little really gets in your way.
There's been requests for comment types to indicate if a comment is a defect, what type (security issue, coding style, etc.), and how severe it is. David and I have talked about this a bit and, while CodeCollaborator and Crucible both support this and some users have asked for it, we feel it goes down the route of too much policy. I think what we may do here is stick with the original plan of waiting for the 2.0 release (which will have extensions) and writing extensions for this sort of functionality. Those companies that need extensive customizations can write or use an appropriate extension. Keeps the Review Board code simple and the experience straightforward for the majority of people without preventing companies who really need this type of support from having it. Christian -- Christian Hammond - [email protected] Review Board - http://www.review-board.org VMware, Inc. - http://www.vmware.com On Sun, Oct 11, 2009 at 8:18 PM, Dan Savilonis <[email protected]> wrote: > > I agree these could be useful features, but you have to be careful not > to delve into the 'policy' realm which will make everyone despise the > tool. > > We also use CodeCollaborator where I work and no one wants to use it > for small reviews because it's a waste of time. It implements this > ping-pong model to the extreme. Every time you finish the review, you > mark it complete. If a single person makes another comment, all > completed reviews are now uncompleted and every other reviewer must go > back and mark the new comment as read, and sign off again. > > It might ultimately be most useful to add customizable user fields to > reviews, instead of something like 'severity' which inevitably won't > meet someone's needs. > > Dan > > On Oct 10, 7:53 pm, Christian Hammond <[email protected]> wrote: > > Hi, > > > > Thanks for your feedback. Comments inline. > > > > > > > > > > > > On Tue, Oct 6, 2009 at 8:17 AM, Biking4Fun <[email protected]> wrote: > > > Only show requests that need your action - Tennis Ball model > > > > > > --------------------------------------------------------------------------- > ----- > > > Currently, a review stays in everyone's view until the requester has > > > marked it submitted, etc. This tends to clutter one's view and makes > > > it unclear what you need to act upon. > > > > > It would be better if ReviewBoard implemented a tennis / ping pong > > > model. > > > > > What I mean by that is the review should only be in someone's view > > > when they need to act on the review. Once the reviewer has commented > > > and/or marked it "Ship It", that is like hitting the ball back to the > > > requester's court. It should no longer appear in that particular > > > reviewer's view, because they have already acted upon it. It should > > > now show up in the requester's view, because someone has commented and > > > perhaps found a problem. > > > > There's a lot of value in this model, but we'd have to be careful with > how > > we implement it. It's not always ideal to only see review requests you > > haven't commented on. There are times when I've only been able to review > > part of a very large change, made that review public, and then went back > to > > it later to review more of it. In cases like these, I'd certainly want to > > see the review request still. > > > > I think what we could do is add a form of filtering to the dashboard, so > you > > could show or hide review requests you've already looked at that haven't > > been updated since you last saw it. > > > > I don't know that anything needs to be done in the requester's view. A > > person generally only has a few review requests out at any given point in > > time. The above filter could just apply to this view as well if it's > really > > needed. > > > > Feel free to file a feature request on this, so we can track it for > future > > releases. > > > > Request Priority > > > > > -------------------- > > > Some code changes are for future releases, a lower priority. Others > > > are for a critical fix, or trying to make a code freeze deadline- a > > > higher priority. There currently isn't a way for a requester to > > > indicate that a particular review is important and needs to be looked > > > at quickly. > > > > We've discussed adding some sort of severity indicator to a review > request. > > Maybe we will eventually, but we're not set on it yet for a couple of > > reasons. > > > > 1) Severity levels can (and will) be abused. Everyone thinks their review > > request is important and unless you're working with Git or something > > similar, you're often blocked until your change goes in. If we give the > > ability to indicate low, medium and high priorities, then most people > will > > probably pick high, in order to get their change looked at. Review > requests > > with lower priorities will probably just end up being ignored, as there > will > > always be higher priority review requests. In the end, this won't do > anyone > > any good. > > > > 2) Severity levels set by the person posting the review request makes > sense > > in corporate environments but often don't in open source projects. If > > someone is submitting a patch to Review Board, for instance, I really > don't > > want them to decide that it's critical to get it in. It makes a lot more > > sense for myself as the developer to decide the priority of it. But then, > > that doesn't really help us any, because if we consider it critical to > get > > in now, we should just review it now. > > > > 3) Severity levels don't communicate *why* it's important. Is it because > > there's a beta coming up that needs that change? Is it blocking the > person? > > Is it blocking somebody else? Is it some emergency security fix? Build > > breakage? I don't think a severity level communicates enough here. > > > > 4) There's already a mechanism for communicating that something is > important > > and needs to be done for some deadline or milestone. Put it in the > summary. > > At VMware, for example, people will sometimes prefix a summary with > "[Beta 1 > > fix]" and everyone working on the product will know that beta 1 is coming > up > > and that that needs to take priority. No need for any special fields in > the > > database, special UI, or anything. > > > > We may reconsider it sometime, but our design philosophy so far has been > to > > keep things pretty straightforward and free of a lot of policy. Many > things > > can be communicated quite well through text, and introducing special UI > just > > means that users now have to learn one more little thing. It also often > > means that, while the feature is good for some people, it won't meet > someone > > else's need/preference, and we'll be asked to expand on it, thus making > it > > more complicated for everyone else. So far, we've worked to keep the > number > > of features like this in the product pretty small. > > > > Hope that made sense. > > > > Christian > > > > -- > > Christian Hammond - [email protected] > > Review Board -http://www.review-board.org > > VMware, Inc. -http://www.vmware.com > > You received this message because you are subscribed to the Google > Groups "reviewboard" group. > To post to this group, send email to [email protected] > To unsubscribe from this group, send email to > [email protected]<reviewboard%[email protected]> > For more options, visit this group at > http://groups.google.com/group/reviewboard?hl=en > -~----------~----~----~----~------~----~------~--~--- > > --~--~---------~--~----~------------~-------~--~----~ Want to help the Review Board project? Donate today at http://www.review-board.org/donate/ Happy user? Let us know at http://www.review-board.org/users/ -~----------~----~----~----~------~----~------~--~--- You received this message because you are subscribed to the Google Groups "reviewboard" group. To post to this group, send email to [email protected] To unsubscribe from this group, send email to [email protected] For more options, visit this group at http://groups.google.com/group/reviewboard?hl=en -~----------~----~----~----~------~----~------~--~---
