Re: keeping track of large number of files
On Fri, Sep 10, 2010 at 09:03:24PM -0700, J Arrizza wrote: There is one more aspect that I think is important. I look at the SCR as the central unit of work, not the changelist/changeset. RB seems to be geared towards one changeset == one code review But from my perspective, the situation is more like: one SCR == one code review That's why I want to put multiple files into a code review and multiple changesets into a code review. An SCR holds a single enhancement or perhaps a well-defined sub-part of that enhancement. It has to be this way since external non-development people consider an enhancement as a single change and track it, report it, and think about it that way. This also means that an SCR becomes a place to store evidence that we've done everything in the development process we are supposed to do according to our formal procedures. If everything is in a single SCR, it becomes much easier to track and to report that all is ok, i.e. the SCR says the RB id is nnn, the RB code review nnn says every code change was reviewed by every reviewer, therefore the enhancement (aka the SCR) has had a complete set of code reviews performed on it. What this implies to RB is it should have the capability to: - add and remove files in a code review - add flags or checkboxes to each file for each reviewer to indicate the file was reviewed. - add flags or checkboxes for every comment to indicate the comment was addressed or re-reviewed by every reviewer. - The original version doesn't change and all subsequent diffs are compared against it. It is the version of code that existed before the work was started on the SCR. In other words, for a given file, the left hand side of the diff never changes, only the right hand side. If a file is updated, I only want to see the latest version. I would be ok with not seeing any of the intermediate versions/diffs. They just confuse the situation for outside observers (e.g. auditors). John On Mon, Aug 30, 2010 at 10:08 AM, J Arrizza cppge...@gmail.com wrote: Christian, Thanks for the response! Have you used Google Reader? In Google Reader, as you scroll through a list of posts, it will mark the ones you've scrolled past (or interacted with) as read. This makes it very easy to see all the posts or just the posts you haven't gotten to yet. I haven't used Google Reader, but I think I know what you mean, i.e. similar to the gmail, outlook etc that bolds the subject line? I think this would be good, but my preference is to make the indication more explicit. Just bolding or italicizing a file name isn't concrete enough since visual indications can be unconsciously ignored or inadvertently applied. In any case, it also has to be un-doable (just like outlook allows you to mark as unread). I could also see allowing easily hiding/showing of a diff. Kind of an expander (not diff chunk expansion) where you could collapse the entire thing down to one line with the file's name. That requires a bit more manual work to keep track of where you are, though. I think this and the above together would be a very powerful UI addition to RB. For example, I could collapse all which would show the entire list of files as a simple list, with the ok/not ok as one of the columns. Then I could very quickly find the file that's missing and review it. Also it's a great visual progress indicator. The caveat, I think, is the collapsed list has to show *all* the files, whether new, deleted, modified, in the current revision or not. That sounds more complicated for implementation but from a user perspective, it's a whole lot of nice-to-have. Of course, if RB could assist me by marking a file as not ok whenever a new comment is applied to it by any reviewer, I wouldn't object! I want to know if it would work for your needs, though. I need assistance in tracking the changes developers are doing wrt to fixes based on reviews. The initial reviews are straightforward, but I need to ensure that they have, in fact, implemented the changes from the reviews and that the updates were also reviewed. Since initial commits are whole files and not just a few diffs, there can be a large number of comments per file and of course a large number of files. It quickly gets out of hand, especially when the updates for the comments are put in a few at a time or by different developers. In short, at the end of this process, I need to be able to state with explicit evidence that: - all the reviewers have reviewed the file by applying some positive indication, either a comment or an OK - all of the comments have been addressed, either by a code update or by a discussion indicating everyone agrees the code is OK as is. - all of the code updates were reviewed - ...and follow that tail recursion on down to the last
Re: keeping track of large number of files
There is one more aspect that I think is important. I look at the SCR as the central unit of work, not the changelist/changeset. RB seems to be geared towards one changeset == one code review But from my perspective, the situation is more like: one SCR == one code review That's why I want to put multiple files into a code review and multiple changesets into a code review. An SCR holds a single enhancement or perhaps a well-defined sub-part of that enhancement. It has to be this way since external non-development people consider an enhancement as a single change and track it, report it, and think about it that way. This also means that an SCR becomes a place to store evidence that we've done everything in the development process we are supposed to do according to our formal procedures. If everything is in a single SCR, it becomes much easier to track and to report that all is ok, i.e. the SCR says the RB id is nnn, the RB code review nnn says every code change was reviewed by every reviewer, therefore the enhancement (aka the SCR) has had a complete set of code reviews performed on it. What this implies to RB is it should have the capability to: - add and remove files in a code review - add flags or checkboxes to each file for each reviewer to indicate the file was reviewed. - add flags or checkboxes for every comment to indicate the comment was addressed or re-reviewed by every reviewer. - The original version doesn't change and all subsequent diffs are compared against it. It is the version of code that existed before the work was started on the SCR. In other words, for a given file, the left hand side of the diff never changes, only the right hand side. If a file is updated, I only want to see the latest version. I would be ok with not seeing any of the intermediate versions/diffs. They just confuse the situation for outside observers (e.g. auditors). John On Mon, Aug 30, 2010 at 10:08 AM, J Arrizza cppge...@gmail.com wrote: Christian, Thanks for the response! Have you used Google Reader? In Google Reader, as you scroll through a list of posts, it will mark the ones you've scrolled past (or interacted with) as read. This makes it very easy to see all the posts or just the posts you haven't gotten to yet. I haven't used Google Reader, but I think I know what you mean, i.e. similar to the gmail, outlook etc that bolds the subject line? I think this would be good, but my preference is to make the indication more explicit. Just bolding or italicizing a file name isn't concrete enough since visual indications can be unconsciously ignored or inadvertently applied. In any case, it also has to be un-doable (just like outlook allows you to mark as unread). I could also see allowing easily hiding/showing of a diff. Kind of an expander (not diff chunk expansion) where you could collapse the entire thing down to one line with the file's name. That requires a bit more manual work to keep track of where you are, though. I think this and the above together would be a very powerful UI addition to RB. For example, I could collapse all which would show the entire list of files as a simple list, with the ok/not ok as one of the columns. Then I could very quickly find the file that's missing and review it. Also it's a great visual progress indicator. The caveat, I think, is the collapsed list has to show *all* the files, whether new, deleted, modified, in the current revision or not. That sounds more complicated for implementation but from a user perspective, it's a whole lot of nice-to-have. Of course, if RB could assist me by marking a file as not ok whenever a new comment is applied to it by any reviewer, I wouldn't object! I want to know if it would work for your needs, though. I need assistance in tracking the changes developers are doing wrt to fixes based on reviews. The initial reviews are straightforward, but I need to ensure that they have, in fact, implemented the changes from the reviews and that the updates were also reviewed. Since initial commits are whole files and not just a few diffs, there can be a large number of comments per file and of course a large number of files. It quickly gets out of hand, especially when the updates for the comments are put in a few at a time or by different developers. In short, at the end of this process, I need to be able to state with explicit evidence that: - all the reviewers have reviewed the file by applying some positive indication, either a comment or an OK - all of the comments have been addressed, either by a code update or by a discussion indicating everyone agrees the code is OK as is. - all of the code updates were reviewed - ...and follow that tail recursion on down to the last turtle :) I can handle these by imposing rules on the developers, but having a tool guide the process is much much better. John (PS the explicit evidence is for
Re: keeping track of large number of files
So far, what we've done is just show a series of deletes for each line in the file, which isn't really a good this file was deleted indicator. In 1.5 RC1, we have the beginnings of support for marking that a file is indeed deleted. Only CVS supports this right now (we needed it to work around a problem with how CVS diffs represented deleted files). I'm hoping in 1.5.x/1.6 to begin supporting this at least for Subversion, Git, and Perforce. Christian -- Christian Hammond - chip...@chipx86.com Review Board - http://www.reviewboard.org VMware, Inc. - http://www.vmware.com On Mon, Aug 30, 2010 at 11:25 AM, Chris Clark chris.cl...@ingres.comwrote: J Arrizza wrote: The caveat, I think, is the collapsed list has to show *all* the files, whether new, deleted, modified, in the current revision or not. That sounds more complicated for implementation but from a user perspective, it's a whole lot of nice-to-have. Does RB have support for files that are deleted in a review? For our private deployment of 1.0.5.1 I ended up faking a diff (in the SCMTool) that has NOTE! reviewer this file has been marked for deletion as the first line. Chris -- Want to help the Review Board project? Donate today at http://www.reviewboard.org/donate/ Happy user? Let us know at http://www.reviewboard.org/users/ -~--~~~~--~~--~--~--- To unsubscribe from this group, send email to reviewboard+unsubscr...@googlegroups.comreviewboard%2bunsubscr...@googlegroups.com 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.reviewboard.org/donate/ Happy user? Let us know at http://www.reviewboard.org/users/ -~--~~~~--~~--~--~--- To unsubscribe from this group, send email to reviewboard+unsubscr...@googlegroups.com For more options, visit this group at http://groups.google.com/group/reviewboard?hl=en
Re: keeping track of large number of files
Christian, Thanks for the response! Have you used Google Reader? In Google Reader, as you scroll through a list of posts, it will mark the ones you've scrolled past (or interacted with) as read. This makes it very easy to see all the posts or just the posts you haven't gotten to yet. I haven't used Google Reader, but I think I know what you mean, i.e. similar to the gmail, outlook etc that bolds the subject line? I think this would be good, but my preference is to make the indication more explicit. Just bolding or italicizing a file name isn't concrete enough since visual indications can be unconsciously ignored or inadvertently applied. In any case, it also has to be un-doable (just like outlook allows you to mark as unread). I could also see allowing easily hiding/showing of a diff. Kind of an expander (not diff chunk expansion) where you could collapse the entire thing down to one line with the file's name. That requires a bit more manual work to keep track of where you are, though. I think this and the above together would be a very powerful UI addition to RB. For example, I could collapse all which would show the entire list of files as a simple list, with the ok/not ok as one of the columns. Then I could very quickly find the file that's missing and review it. Also it's a great visual progress indicator. The caveat, I think, is the collapsed list has to show *all* the files, whether new, deleted, modified, in the current revision or not. That sounds more complicated for implementation but from a user perspective, it's a whole lot of nice-to-have. Of course, if RB could assist me by marking a file as not ok whenever a new comment is applied to it by any reviewer, I wouldn't object! I want to know if it would work for your needs, though. I need assistance in tracking the changes developers are doing wrt to fixes based on reviews. The initial reviews are straightforward, but I need to ensure that they have, in fact, implemented the changes from the reviews and that the updates were also reviewed. Since initial commits are whole files and not just a few diffs, there can be a large number of comments per file and of course a large number of files. It quickly gets out of hand, especially when the updates for the comments are put in a few at a time or by different developers. In short, at the end of this process, I need to be able to state with explicit evidence that: - all the reviewers have reviewed the file by applying some positive indication, either a comment or an OK - all of the comments have been addressed, either by a code update or by a discussion indicating everyone agrees the code is OK as is. - all of the code updates were reviewed - ...and follow that tail recursion on down to the last turtle :) I can handle these by imposing rules on the developers, but having a tool guide the process is much much better. John (PS the explicit evidence is for the FDA and our internal quality folks but is applicable to any regulated environment, e.g. FAA, DOD, others?) On Sun, Aug 29, 2010 at 4:05 PM, Christian Hammond chip...@chipx86.comwrote: Hi John, Have you used Google Reader? In Google Reader, as you scroll through a list of posts, it will mark the ones you've scrolled past (or interacted with) as read. This makes it very easy to see all the posts or just the posts you haven't gotten to yet. Would something like that suffice? Visually indicate and internally store which diffs you've read vs. the ones you haven't, and allow for quick filtering of which ones you have or haven't seen. I would personally like a model like this, though I think we'd have to play with it a bit. I want to know if it would work for your needs, though. I could also see allowing easily hiding/showing of a diff. Kind of an expander (not diff chunk expansion) where you could collapse the entire thing down to one line with the file's name. That requires a bit more manual work to keep track of where you are, though. Christian -- Christian Hammond - chip...@chipx86.com Review Board - http://www.reviewboard.org VMware, Inc. - http://www.vmware.com On Fri, Aug 27, 2010 at 9:34 AM, J Arrizza cppge...@gmail.com wrote: Christian, I agree with you as well. However, when someone does publish a massive review, I have no recourse. Here are some suggestions (and only suggestions, meant only to get some conversation going): - allow me to put a comment at a file level (i.e. not on a line number within the file). I can use that to indicate OK. - allow me to put a status on a file. A status is just an ok/not ok indication, no text can be entered. Then allow the file status to cross revisions, i.e. whenever I view any revision, I get a complete list of files from all revisions. The only diffs I see are for the current revision, but I can see all statuses for all files. - allow me to put a status on a file, plus have RB keep track of the total number of comments for that file across
Re: keeping track of large number of files
Hi John, Have you used Google Reader? In Google Reader, as you scroll through a list of posts, it will mark the ones you've scrolled past (or interacted with) as read. This makes it very easy to see all the posts or just the posts you haven't gotten to yet. Would something like that suffice? Visually indicate and internally store which diffs you've read vs. the ones you haven't, and allow for quick filtering of which ones you have or haven't seen. I would personally like a model like this, though I think we'd have to play with it a bit. I want to know if it would work for your needs, though. I could also see allowing easily hiding/showing of a diff. Kind of an expander (not diff chunk expansion) where you could collapse the entire thing down to one line with the file's name. That requires a bit more manual work to keep track of where you are, though. Christian -- Christian Hammond - chip...@chipx86.com Review Board - http://www.reviewboard.org VMware, Inc. - http://www.vmware.com On Fri, Aug 27, 2010 at 9:34 AM, J Arrizza cppge...@gmail.com wrote: Christian, I agree with you as well. However, when someone does publish a massive review, I have no recourse. Here are some suggestions (and only suggestions, meant only to get some conversation going): - allow me to put a comment at a file level (i.e. not on a line number within the file). I can use that to indicate OK. - allow me to put a status on a file. A status is just an ok/not ok indication, no text can be entered. Then allow the file status to cross revisions, i.e. whenever I view any revision, I get a complete list of files from all revisions. The only diffs I see are for the current revision, but I can see all statuses for all files. - allow me to put a status on a file, plus have RB keep track of the total number of comments for that file across all revisions. - have a status (ok/not ok) at the file level, one for each revieiwer. Initially the status is not ok. A new comment on any revision for that file resets the file status to not ok. All reviewers can then go in, look at the new comment and/or code fix, and reset the status (for them) to ok. - allow me to remove a diff/file from the current review. - allow me to move a diff/file to another, new, review. - give me full GUI support for splitting up an existing review (whatever that means! :) Are any of these easy to implement given the current RB architecture? John On Thu, Aug 26, 2010 at 2:34 AM, Christian Hammond chip...@chipx86.comwrote: I'll second this. It really is beneficial to try to break up the changes more. Preferably into logical changes (such as code to implement one part of the functionality rather than an entire feature, or doing code cleanups in a separate change), though if you're past the point where that makes sense, even breaking it up into file paths (potentially based on who is most familiar with certain groups of files) would probably help. I've seen this a lot too, where really large changes of mine have sat around because they're too long to review. Not only do people avoid them initially (since it requires such a time commitment), but the feedback isn't as good as it could be. Smaller changes receive better review and can get into the product sooner. Christian -- Want to help the Review Board project? Donate today at http://www.reviewboard.org/donate/ Happy user? Let us know at http://www.reviewboard.org/users/ -~--~~~~--~~--~--~--- To unsubscribe from this group, send email to reviewboard+unsubscr...@googlegroups.comreviewboard%2bunsubscr...@googlegroups.com 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.reviewboard.org/donate/ Happy user? Let us know at http://www.reviewboard.org/users/ -~--~~~~--~~--~--~--- To unsubscribe from this group, send email to reviewboard+unsubscr...@googlegroups.com For more options, visit this group at http://groups.google.com/group/reviewboard?hl=en
Re: keeping track of large number of files
Christian, I agree with you as well. However, when someone does publish a massive review, I have no recourse. Here are some suggestions (and only suggestions, meant only to get some conversation going): - allow me to put a comment at a file level (i.e. not on a line number within the file). I can use that to indicate OK. - allow me to put a status on a file. A status is just an ok/not ok indication, no text can be entered. Then allow the file status to cross revisions, i.e. whenever I view any revision, I get a complete list of files from all revisions. The only diffs I see are for the current revision, but I can see all statuses for all files. - allow me to put a status on a file, plus have RB keep track of the total number of comments for that file across all revisions. - have a status (ok/not ok) at the file level, one for each revieiwer. Initially the status is not ok. A new comment on any revision for that file resets the file status to not ok. All reviewers can then go in, look at the new comment and/or code fix, and reset the status (for them) to ok. - allow me to remove a diff/file from the current review. - allow me to move a diff/file to another, new, review. - give me full GUI support for splitting up an existing review (whatever that means! :) Are any of these easy to implement given the current RB architecture? John On Thu, Aug 26, 2010 at 2:34 AM, Christian Hammond chip...@chipx86.comwrote: I'll second this. It really is beneficial to try to break up the changes more. Preferably into logical changes (such as code to implement one part of the functionality rather than an entire feature, or doing code cleanups in a separate change), though if you're past the point where that makes sense, even breaking it up into file paths (potentially based on who is most familiar with certain groups of files) would probably help. I've seen this a lot too, where really large changes of mine have sat around because they're too long to review. Not only do people avoid them initially (since it requires such a time commitment), but the feedback isn't as good as it could be. Smaller changes receive better review and can get into the product sooner. Christian -- Want to help the Review Board project? Donate today at http://www.reviewboard.org/donate/ Happy user? Let us know at http://www.reviewboard.org/users/ -~--~~~~--~~--~--~--- To unsubscribe from this group, send email to reviewboard+unsubscr...@googlegroups.com For more options, visit this group at http://groups.google.com/group/reviewboard?hl=en
Re: keeping track of large number of files
I'll second this. It really is beneficial to try to break up the changes more. Preferably into logical changes (such as code to implement one part of the functionality rather than an entire feature, or doing code cleanups in a separate change), though if you're past the point where that makes sense, even breaking it up into file paths (potentially based on who is most familiar with certain groups of files) would probably help. I've seen this a lot too, where really large changes of mine have sat around because they're too long to review. Not only do people avoid them initially (since it requires such a time commitment), but the feedback isn't as good as it could be. Smaller changes receive better review and can get into the product sooner. Christian -- Christian Hammond - chip...@chipx86.com Review Board - http://www.reviewboard.org VMware, Inc. - http://www.vmware.com On Tue, Aug 24, 2010 at 6:48 AM, Scott Quesnelle scott.quesne...@gmail.comwrote: John, This isn't a software answer, but if the changes is that large, then we encourage the author to try and split it into pieces which provide a part of the whole solution. I.e a review of the server side change, another with all the database related changes, and a last one with the client side changes. This helps reviewers with being able to concentrate on the review. Many studies show that if a review is too large (generally takes more then 1 1/2 hours to go through) then the quality of review comments drop. Reviewing code requires a high level of concentration and that can only be maintained for so long. Scott On Mon, Aug 23, 2010 at 10:51 PM, J Arrizza cppge...@gmail.com wrote: Hi, When initial code is put into RB or a large change is put in, there can be many files (say 50). Can't really review that in one sitting so there needs to be a way for a reviewer to keep track of which files he's finished with (e.g. ok with file xyz.cpp). I see that an issue was raised for this already (by Ben Hollis) Issue 1772 http://code.google.com/p/reviewboard/issues/detail?id=1772: Allow reviewers to check off files they've looked at In the meantime, has any one come up with a good workaround? I've thought of just putting a comment that says OK, but that could get messy especially with email notifications... John -- Want to help the Review Board project? Donate today at http://www.reviewboard.org/donate/ Happy user? Let us know at http://www.reviewboard.org/users/ -~--~~~~--~~--~--~--- To unsubscribe from this group, send email to reviewboard+unsubscr...@googlegroups.comreviewboard%2bunsubscr...@googlegroups.com 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.reviewboard.org/donate/ Happy user? Let us know at http://www.reviewboard.org/users/ -~--~~~~--~~--~--~--- To unsubscribe from this group, send email to reviewboard+unsubscr...@googlegroups.comreviewboard%2bunsubscr...@googlegroups.com 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.reviewboard.org/donate/ Happy user? Let us know at http://www.reviewboard.org/users/ -~--~~~~--~~--~--~--- To unsubscribe from this group, send email to reviewboard+unsubscr...@googlegroups.com For more options, visit this group at http://groups.google.com/group/reviewboard?hl=en
Re: keeping track of large number of files
John, This isn't a software answer, but if the changes is that large, then we encourage the author to try and split it into pieces which provide a part of the whole solution. I.e a review of the server side change, another with all the database related changes, and a last one with the client side changes. This helps reviewers with being able to concentrate on the review. Many studies show that if a review is too large (generally takes more then 1 1/2 hours to go through) then the quality of review comments drop. Reviewing code requires a high level of concentration and that can only be maintained for so long. Scott On Mon, Aug 23, 2010 at 10:51 PM, J Arrizza cppge...@gmail.com wrote: Hi, When initial code is put into RB or a large change is put in, there can be many files (say 50). Can't really review that in one sitting so there needs to be a way for a reviewer to keep track of which files he's finished with (e.g. ok with file xyz.cpp). I see that an issue was raised for this already (by Ben Hollis) Issue 1772 http://code.google.com/p/reviewboard/issues/detail?id=1772: Allow reviewers to check off files they've looked at In the meantime, has any one come up with a good workaround? I've thought of just putting a comment that says OK, but that could get messy especially with email notifications... John -- Want to help the Review Board project? Donate today at http://www.reviewboard.org/donate/ Happy user? Let us know at http://www.reviewboard.org/users/ -~--~~~~--~~--~--~--- To unsubscribe from this group, send email to reviewboard+unsubscr...@googlegroups.comreviewboard%2bunsubscr...@googlegroups.com 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.reviewboard.org/donate/ Happy user? Let us know at http://www.reviewboard.org/users/ -~--~~~~--~~--~--~--- To unsubscribe from this group, send email to reviewboard+unsubscr...@googlegroups.com For more options, visit this group at http://groups.google.com/group/reviewboard?hl=en
