1277: Comment Types
- Fixed
- Review Board
| cybr****@gmai***** (Google Code) (Is this you? Claim this profile.) | |
| May 6, 2011 | |
| 946, 1941, 1951, 2000, 2040 |
board.org/
What version are you running?
ReviewBoard-1.1alpha1.dev_r2026
What's the URL of the page this enhancement relates to, if any?
N/A
Describe the enhancement and the motivation for it.
ENHANCEMENT DESCRIPTION:
Provide a drop-down box for "Comment Type". By default, this would contain
3 items:
- NOTE (implies it may not be an issue)
- SUGGESTION (a minor suggestion, can be ignored if necessary)
- ISSUE (an issue that should be looked at before shipping)
In the configuration page, provide the following:
- Allow users to turn this feature ON (if they so wish)
- Allow users to enter custom comment types. If this feature is not turned
on and visible from the UI, then all comment types may be entered under
Note by default.
MOTIVATION:
- METRICS! We need to be able to collect metrics in order to ensure the
continuous improvement of the code review process.
- Having comment types would really help, because it would help us achieve:
-- Categorization of comments: Is this an issue that will result in bad
performance or poor security? AND/OR
-- Severity of comments:
--- Some comments are actually compliments. ("Good job!")
--- Other comments are notes ("it would be nice if in the future you did it
this way...")
What operating system are you using? What browser?
Linux/Firefox
Please provide any additional information below.
Any decent software development team/company needs to collect metrics in
order to achieve continuous improvement. For those companies where
processes are important, this feature would be turned on and it would allow
us to track metrics like:
-- How many "serious" issues were discovered per 1000 lines of code during
review?
-- How does this number compare to 1 year ago.
-- How many "serious" issues per reviewer?
-- How many serious issues per submitter?
-- What types of issues are the most? If Performance issues are always
cropping up, then perhaps we could do a training on coding for better
performance.
While the entire list of metrics is not definitive above, some of them
would be very useful to have.
I realize that not all companies have the same workflow, so there are two
simple things that I have suggested (if this feature is not universally
useful):
- The initial list of issues is very generic (allowing people to customize)
- The feature can be turned off/on, for those teams that don't use it.
We're having some discussions and debates about how far to go with this feature. We have absolutely no problem with adding a "Defect" checkbox, but are wondering if we really do want to add severity and comment type. I think really, we need further convincing. I agree that the data is interesting and I can see the argument for knowing how many performance issues are seen, coding style issues, etc. This does of course depend on people actually being good about setting these. I'm wondering how you see them being used, though. We've generally tried to keep Review Board pretty process-free, keeping it simple and light-weight, but this would certainly add a lot more process to reviewing code. Historically, we've encouraged people to use their words for such things instead of doing a drop-down style code review. It's easy to type "This will hinder performance due to XYZ" and generally makes communication about such things easier. We could require a comment regardless. What I wonder then is whether people will be good about making a comment *and* selecting from drop-downs for the severity and type. This data is meaningless if it's not consistent. If we provide all this, the data will need to be used in some way. I'd like to know how you see it being used. If we implement this, we'd need to know how users would need to use the data, or it'd end up being useful for only one group of people or another. I'm also trying to decide if we should use a drop-down UI or not. Some people wnat severity *and* comment type, but should both be available? Should comment type imply a severity level? Does it even make sense to have a severity level? Should the types be administrator-provided, or should users be able to create their own as they choose? If we add this, I'd like it to be *really* useful for more than just metrics. For example, filtering comments in such a way to tackle the big problems before going after coding style issues. I'd like to see some more uses for it though.
-
+ NeedInfo -
+ Component-DiffViewer + Component-Reviews
Here is a use case for Severity. It is Thursday, 5:00 pm. You must submit your code by Friday according to the project plan so that QA can test it at least 1 full day and hopefully there are no problems, so that a release can be done on Monday. You submit a 2000-line changeset. It gets reviewed by 5 people and overall you have 30 comments to deal with. It is clear that by Monday, you will not be able to deal with all 20. How do you know which one to focus on. Some issues may be minor, some may be major. All of them are "defects". But you need to triage and find the most pressing ones. The defects need to be categorized and labeled. If they had some visible label, then I would scroll down through the review and focus on the 10 most critical defects. The other defects, would get reported to the management and if the management accepts the risk, then we could add a "//FIXME:" tag wherever there is an issue in the code and commit it. The defect type and severity should be prominently visible in the review screen.
I think that while this is a good idea in spirit, I'm not sure it would work in practice. I don't think severity is useful in the case of code reviews in the same way that severity isn't all that useful in the case of bugs (wait for the explanation). In the case of bugs, in most cases, there are a few people who have a the big picture of the product and set the relative severity of the bugs. In this case however, it sounds like the commenter would set the priority. That's probably not a good way to go. I do like the idea of having "comment types" - a defect checkbox, or a checkbox that additionally says that security might be a concern. This classifies them without priority. I think it's up to the developer to read _all_ of the comments and then prioritize and address them.
-
- NeedInfo + New
Just to add my 2 cents. Management needs to be convinced that reviews are 'useful'. So we need some metrics that could show that reviews are finding problems. I'd be happy with a 3 level system: Major, Minor, Info. And by counting the number of Major and Minor, I could argue that reviews are finding problems that needed to be fixed. I don't see the need for a type, but I understand opinions may vary. My point is more to be able to make a difference between 'require a fix' and 'for info'. My concern is about adding some red tape, and breaking the flow. We could set a default severity, but then a lot of people would just keep the default. We could force a choice, but then, it hinders the flow. I also would like the severity to be customizable. Though my preference would be 3 choices, we're using a defect management system that uses severities 1 to 5. So even if it's an overkill, it may be easier to use the same scale, as people are used to it.