The word "defect" is pretty loaded. It is generally a term for a bug and is first reported by a QA department, not a code review. Then the workflows for dealing with prioritizing them and acting upon the vary wildly.
In light of that, using the word "defect" means the first question you'll get is going to be "how do I push these to Bugzilla/Trac/DevTrack/FogBugz/Jira?" That question sucks, unless you plan to implement all of the functionality of those systems, which I imagine you aren't planning right now.
So... avoid the word "defect".. IMO.
In our RB process, a chunk of code that passes through a review successfully with known issues gets one of two things:
a) An immediate review/checkin afterwards of the fix. b) A "TODO:" comment in the code.
The word I prefer for what I think you're trying to do here is "Blocker". In our workflow, blockers _must_ be fixed or the code can't be checked in. So there's no such thing as changing a blocker to a "TODO", like the "defect passed" functionality in your proposal. A developer needs to argue their case in person for that kind of override.
We implemented the functionality I described just now and put it on Github. You are more than welcome to pick it up and run with it, though based on an older revision of the code at this point.
Excellent - exactly what I wanted: some wisdom from the trenches. :D
> The word "defect" is pretty loaded. It is generally a term for a bug and is
> first reported by a QA department, not a code review. Then the workflows
> for dealing with prioritizing them and acting upon the vary wildly.
You're absolutely right. And I like Blocker. A lot. It's simple.
And it already fits in - at least when I've used RB, if I can't get a
review request through, I say that I'm blocked by it.
If there are no objections, I think I might go that route.
> In our workflow, blockers _must_ be fixed or the code can't be checked in.
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"?
> You are more than welcome to pick it up and run with it, though based on an
> older revision of the code at this point.
I'll take a peek. Thanks for the insight! Let me know if you think
of anything more.
-Mike
On Jul 10, 9:32 pm, Chris Trimble <tri...@gmail.com> wrote:
> The word "defect" is pretty loaded. It is generally a term for a bug and is
> first reported by a QA department, not a code review. Then the workflows
> for dealing with prioritizing them and acting upon the vary wildly.
> In light of that, using the word "defect" means the first question you'll
> get is going to be "how do I push these to
> Bugzilla/Trac/DevTrack/FogBugz/Jira?" That question sucks, unless you plan
> to implement all of the functionality of those systems, which I imagine you
> aren't planning right now.
> So... avoid the word "defect".. IMO.
> In our RB process, a chunk of code that passes through a review successfully
> with known issues gets one of two things:
> a) An immediate review/checkin afterwards of the fix.
> b) A "TODO:" comment in the code.
> The word I prefer for what I think you're trying to do here is "Blocker".
> In our workflow, blockers _must_ be fixed or the code can't be checked in.
> So there's no such thing as changing a blocker to a "TODO", like the
> "defect passed" functionality in your proposal. A developer needs to argue
> their case in person for that kind of override.
> We implemented the functionality I described just now and put it on Github.
> You are more than welcome to pick it up and run with it, though based on an
> older revision of the code at this point.
I've had some more thoughts about the naming. Initially, I thought
"Blocker" was the right fit, but I'm starting to have second thoughts.
Thinking about it, "blocker" sounds a little adversarial. Almost
defensive. A blocker is an impediment.
This might not sound important, but code review, whether we like it or
not, is a social activity. I believe the less confrontational, and
the more *collaborative*, the better.
So how about this - instead of a Defect, or a Blocker, how about an
Issue?
An Issue can be raised. An Issue can be Resolved. An Issue can be
Dropped.
Would that work? And anybody else care to chime in?
-Mike
On Jul 10, 10:42 pm, Mike Conley <mike.d.con...@gmail.com> wrote:
> Excellent - exactly what I wanted: some wisdom from the trenches. :D
> > The word "defect" is pretty loaded. It is generally a term for a bug and is
> > first reported by a QA department, not a code review. Then the workflows
> > for dealing with prioritizing them and acting upon the vary wildly.
> You're absolutely right. And I like Blocker. A lot. It's simple.
> And it already fits in - at least when I've used RB, if I can't get a
> review request through, I say that I'm blocked by it.
> If there are no objections, I think I might go that route.
> > In our workflow, blockers _must_ be fixed or the code can't be checked in.
> 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"?
> > You are more than welcome to pick it up and run with it, though based on an
> > older revision of the code at this point.
> I'll take a peek. Thanks for the insight! Let me know if you think
> of anything more.
> -Mike
> On Jul 10, 9:32 pm, Chris Trimble <tri...@gmail.com> wrote:
> > Cool project Mike!
> > A few thoughts from the trenches...
> > The word "defect" is pretty loaded. It is generally a term for a bug and is
> > first reported by a QA department, not a code review. Then the workflows
> > for dealing with prioritizing them and acting upon the vary wildly.
> > In light of that, using the word "defect" means the first question you'll
> > get is going to be "how do I push these to
> > Bugzilla/Trac/DevTrack/FogBugz/Jira?" That question sucks, unless you plan
> > to implement all of the functionality of those systems, which I imagine you
> > aren't planning right now.
> > So... avoid the word "defect".. IMO.
> > In our RB process, a chunk of code that passes through a review successfully
> > with known issues gets one of two things:
> > a) An immediate review/checkin afterwards of the fix.
> > b) A "TODO:" comment in the code.
> > The word I prefer for what I think you're trying to do here is "Blocker".
> > In our workflow, blockers _must_ be fixed or the code can't be checked in.
> > So there's no such thing as changing a blocker to a "TODO", like the
> > "defect passed" functionality in your proposal. A developer needs to argue
> > their case in person for that kind of override.
> > We implemented the functionality I described just now and put it on Github.
> > You are more than welcome to pick it up and run with it, though based on an
> > older revision of the code at this point.
On Sun, Jul 11, 2010 at 8:25 PM, Mike Conley <mike.d.con...@gmail.com>wrote:
> So how about this - instead of a Defect, or a Blocker, how about an > Issue?
Sounds good to me. Whatever term you think accurately indicates "we need to fix this before a checkin" but also doesn't read as "needs to be in our bug database."
Or we could just make that term something one can set themselves in the config file. I could set it up to call them "Thunderdomes" for example. Two reviews enter, one checkin leaves.
On Sat, Jul 10, 2010 at 7:42 PM, Mike Conley <mike.d.con...@gmail.com>wrote:
> 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"?
Not sure we have a defined term for it. "Resolved" sounds good though.
On Sunday, July 11, 2010, Mike Conley <mike.d.con...@gmail.com> wrote: > So how about this - instead of a Defect, or a Blocker, how about an > Issue?
> An Issue can be raised. An Issue can be Resolved. An Issue can be > Dropped.
> Would that work? And anybody else care to chime
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 tracker.
> On Sunday, July 11, 2010, Mike Conley <mike.d.con...@gmail.com> wrote:
> > So how about this - instead of a Defect, or a Blocker, how about an
> > Issue?
> > An Issue can be raised. An Issue can be Resolved. An Issue can be
> > Dropped.
> > Would that work? And anybody else care to chime
> 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
> tracker.
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.
On Mon, Jul 12, 2010 at 5:55 AM, Mike Conley <mike.d.con...@gmail.com> wrote: > Ok, more cracks at it:
> Problem > Problem Solved > Not a Problem
> Concern > Concern Addressed > Concern Dropped
> Point Raised > Point Taken > Point Dropped
> Catch > Caught > Dropped
> Favourites?
> -Mike
> On Jul 12, 12:31 am, Christian Hammond <chip...@chipx86.com> wrote: >> On Sunday, July 11, 2010, Mike Conley <mike.d.con...@gmail.com> wrote: >> > So how about this - instead of a Defect, or a Blocker, how about an >> > Issue?
>> > An Issue can be raised. An Issue can be Resolved. An Issue can be >> > Dropped.
>> > Would that work? And anybody else care to chime
>> 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 >> tracker.
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.\
My personal favorite is "Issue", but no matter what we call it, we're going to get the "how do I get these issues into my bug tracker" question, to which our answer is that "these aren't product bugs, they're requested code changes that people want to track." As long as we emphasize that it's a new workflow element instead of something that they already have, I think we'll be fine.
-David
On Mon, Jul 12, 2010 at 4:23 AM, Stephen Gallagher
> 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.\
Ok, I'm fine with that. Unless there are further objections, Issue it
is.
Hey David and Stephen - thank you both for the feedback and
suggestions!
> 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.
Part of my task is to leave the Issue tracking/reporting open for
extension so that you *can* have those dropdowns if you need them.
I'm working on a project with Review Board where we'll probably want
the same type of dropdowns (specifically - issue type and severity).
Once my work getting the bare-bones Issue business in, I'll build that
extension, and then if you'd like, you can modify that extension to
suit your purposes.
Because in the end, if we were to toss those dropdowns in, someone
would undoubtably complain that they don't *exactly* fit their
workflow, etc...and we'd be sunk. So I'm adopting a strategy of uber-
simple + extensibility. Everybody (hopefully) wins! :D
Thanks everybody for all of your feedback, and for letting me think
out loud. :D
-Mike
On Jul 12, 6:10 pm, David Trowbridge <trowb...@gmail.com> wrote:
> My personal favorite is "Issue", but no matter what we call it, we're
> going to get the "how do I get these issues into my bug tracker"
> question, to which our answer is that "these aren't product bugs,
> they're requested code changes that people want to track." As long as
> we emphasize that it's a new workflow element instead of something
> that they already have, I think we'll be fine.
> -David
> On Mon, Jul 12, 2010 at 4:23 AM, Stephen Gallagher
> <step...@gallagherhome.com> wrote:
> > On 7/12/2010 12:55 AM, Mike Conley wrote:
> >> Ok, more cracks at it:
> >> Problem
> >> Problem Solved
> >> Not a Problem
> >> Point Raised
> >> Point Taken
> >> Point Dropped
> >> Catch
> >> Caught
> >> Dropped
> >> Favourites?
> > 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.\
Regarding choice of vocabulary, I don't think you'll be able to satisfy everyone. If anyone used Trac, the admin interface makes it easy to change the vocabulary for priorities, resolutions, severities, and ticket types. I also liked Stephan's workflow.
I think this is a great addition to the core by the way.
Daniel
On Tue, Jul 13, 2010 at 4:01 PM, Mike Conley <mike.d.con...@gmail.com>wrote:
> Ok, I'm fine with that. Unless there are further objections, Issue it > is.
> Hey David and Stephen - thank you both for the feedback and > suggestions!
> > 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.
> Part of my task is to leave the Issue tracking/reporting open for > extension so that you *can* have those dropdowns if you need them.
> I'm working on a project with Review Board where we'll probably want > the same type of dropdowns (specifically - issue type and severity). > Once my work getting the bare-bones Issue business in, I'll build that > extension, and then if you'd like, you can modify that extension to > suit your purposes.
> Because in the end, if we were to toss those dropdowns in, someone > would undoubtably complain that they don't *exactly* fit their > workflow, etc...and we'd be sunk. So I'm adopting a strategy of uber- > simple + extensibility. Everybody (hopefully) wins! :D
> Thanks everybody for all of your feedback, and for letting me think > out loud. :D
> -Mike
> On Jul 12, 6:10 pm, David Trowbridge <trowb...@gmail.com> wrote: > > My personal favorite is "Issue", but no matter what we call it, we're > > going to get the "how do I get these issues into my bug tracker" > > question, to which our answer is that "these aren't product bugs, > > they're requested code changes that people want to track." As long as > > we emphasize that it's a new workflow element instead of something > > that they already have, I think we'll be fine.
> > -David
> > On Mon, Jul 12, 2010 at 4:23 AM, Stephen Gallagher
> > <step...@gallagherhome.com> wrote: > > > On 7/12/2010 12:55 AM, Mike Conley wrote:
> > >> Ok, more cracks at it:
> > >> Problem > > >> Problem Solved > > >> Not a Problem
> > >> Point Raised > > >> Point Taken > > >> Point Dropped
> > >> Catch > > >> Caught > > >> Dropped
> > >> Favourites?
> > > 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.\
This idea is blind alley in my opinion. We have very good and many
stable, grate defect tracking tools and we love it. If I undestand You
try implement very basic defect tracking functionality on the top of
Review Board. This remind me Smarty (PHP Template system) - if we have
already many template system on the top of PHP lets implement PHP in
template system :| Smarty in my opinion is a blind alley and worst
architecture idea for template system and If I understand Your idea
clearly You do something similar.
Instead integration with external tracking tools will be good idea.
Greetings from Poland!
--
Jan Koprowski
On 14 Lip, 01:46, Daniel Swid <da.s...@gmail.com> wrote:
> Regarding choice of vocabulary, I don't think you'll be able to satisfy
> everyone. If anyone used Trac, the admin interface makes it easy to change
> the vocabulary for priorities, resolutions, severities, and ticket types. I
> also liked Stephan's workflow.
> I think this is a great addition to the core by the way.
> Daniel
> On Tue, Jul 13, 2010 at 4:01 PM, Mike Conley <mike.d.con...@gmail.com>wrote:
> > Ok, I'm fine with that. Unless there are further objections, Issue it
> > is.
> > Hey David and Stephen - thank you both for the feedback and
> > suggestions!
> > > 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.
> > Part of my task is to leave the Issue tracking/reporting open for
> > extension so that you *can* have those dropdowns if you need them.
> > I'm working on a project with Review Board where we'll probably want
> > the same type of dropdowns (specifically - issue type and severity).
> > Once my work getting the bare-bones Issue business in, I'll build that
> > extension, and then if you'd like, you can modify that extension to
> > suit your purposes.
> > Because in the end, if we were to toss those dropdowns in, someone
> > would undoubtably complain that they don't *exactly* fit their
> > workflow, etc...and we'd be sunk. So I'm adopting a strategy of uber-
> > simple + extensibility. Everybody (hopefully) wins! :D
> > Thanks everybody for all of your feedback, and for letting me think
> > out loud. :D
> > -Mike
> > On Jul 12, 6:10 pm, David Trowbridge <trowb...@gmail.com> wrote:
> > > My personal favorite is "Issue", but no matter what we call it, we're
> > > going to get the "how do I get these issues into my bug tracker"
> > > question, to which our answer is that "these aren't product bugs,
> > > they're requested code changes that people want to track." As long as
> > > we emphasize that it's a new workflow element instead of something
> > > that they already have, I think we'll be fine.
> > > -David
> > > On Mon, Jul 12, 2010 at 4:23 AM, Stephen Gallagher
> > > <step...@gallagherhome.com> wrote:
> > > > On 7/12/2010 12:55 AM, Mike Conley wrote:
> > > >> Ok, more cracks at it:
> > > >> Problem
> > > >> Problem Solved
> > > >> Not a Problem
> > > >> Point Raised
> > > >> Point Taken
> > > >> Point Dropped
> > > >> Catch
> > > >> Caught
> > > >> Dropped
> > > >> Favourites?
> > > > 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.\
As I said, this isn't about defect tracking for the *product*, which I agree that there are many great tools for. This is about providing a way to aggregate and track defects in the change -- it's often easy to miss fixing one thing when you're trying to respond to dozens of requested changes, and improving the workflow for this is long overdue. Most other code review tools have such a workflow.
On Sun, Jul 18, 2010 at 11:00 PM, Jan Koprowski <jan.koprow...@gmail.com> wrote: > Hi,
> This idea is blind alley in my opinion. We have very good and many > stable, grate defect tracking tools and we love it. If I undestand You > try implement very basic defect tracking functionality on the top of > Review Board. This remind me Smarty (PHP Template system) - if we have > already many template system on the top of PHP lets implement PHP in > template system :| Smarty in my opinion is a blind alley and worst > architecture idea for template system and If I understand Your idea > clearly You do something similar. > Instead integration with external tracking tools will be good idea.
> Greetings from Poland! > -- > Jan Koprowski
> On 14 Lip, 01:46, Daniel Swid <da.s...@gmail.com> wrote: >> Regarding choice of vocabulary, I don't think you'll be able to satisfy >> everyone. If anyone used Trac, the admin interface makes it easy to change >> the vocabulary for priorities, resolutions, severities, and ticket types. I >> also liked Stephan's workflow.
>> I think this is a great addition to the core by the way.
>> Daniel
>> On Tue, Jul 13, 2010 at 4:01 PM, Mike Conley <mike.d.con...@gmail.com>wrote:
>> > Ok, I'm fine with that. Unless there are further objections, Issue it >> > is.
>> > Hey David and Stephen - thank you both for the feedback and >> > suggestions!
>> > > 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.
>> > Part of my task is to leave the Issue tracking/reporting open for >> > extension so that you *can* have those dropdowns if you need them.
>> > I'm working on a project with Review Board where we'll probably want >> > the same type of dropdowns (specifically - issue type and severity). >> > Once my work getting the bare-bones Issue business in, I'll build that >> > extension, and then if you'd like, you can modify that extension to >> > suit your purposes.
>> > Because in the end, if we were to toss those dropdowns in, someone >> > would undoubtably complain that they don't *exactly* fit their >> > workflow, etc...and we'd be sunk. So I'm adopting a strategy of uber- >> > simple + extensibility. Everybody (hopefully) wins! :D
>> > Thanks everybody for all of your feedback, and for letting me think >> > out loud. :D
>> > -Mike
>> > On Jul 12, 6:10 pm, David Trowbridge <trowb...@gmail.com> wrote: >> > > My personal favorite is "Issue", but no matter what we call it, we're >> > > going to get the "how do I get these issues into my bug tracker" >> > > question, to which our answer is that "these aren't product bugs, >> > > they're requested code changes that people want to track." As long as >> > > we emphasize that it's a new workflow element instead of something >> > > that they already have, I think we'll be fine.
>> > > -David
>> > > On Mon, Jul 12, 2010 at 4:23 AM, Stephen Gallagher
>> > > <step...@gallagherhome.com> wrote: >> > > > On 7/12/2010 12:55 AM, Mike Conley wrote:
>> > > >> Ok, more cracks at it:
>> > > >> Problem >> > > >> Problem Solved >> > > >> Not a Problem
>> > > > 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.\
On Mon, Jul 19, 2010 at 8:15 AM, David Trowbridge <trowb...@gmail.com> wrote: > As I said, this isn't about defect tracking for the *product*, which I > agree that there are many great tools for. This is about providing a > way to aggregate and track defects in the change -- it's often easy to > miss fixing one thing when you're trying to respond to dozens of > requested changes, and improving the workflow for this is long > overdue. Most other code review tools have such a workflow.
> -David
> On Sun, Jul 18, 2010 at 11:00 PM, Jan Koprowski <jan.koprow...@gmail.com> wrote: >> Hi,
>> This idea is blind alley in my opinion. We have very good and many >> stable, grate defect tracking tools and we love it. If I undestand You >> try implement very basic defect tracking functionality on the top of >> Review Board. This remind me Smarty (PHP Template system) - if we have >> already many template system on the top of PHP lets implement PHP in >> template system :| Smarty in my opinion is a blind alley and worst >> architecture idea for template system and If I understand Your idea >> clearly You do something similar. >> Instead integration with external tracking tools will be good idea.
>> Greetings from Poland! >> -- >> Jan Koprowski
>> On 14 Lip, 01:46, Daniel Swid <da.s...@gmail.com> wrote: >>> Regarding choice of vocabulary, I don't think you'll be able to satisfy >>> everyone. If anyone used Trac, the admin interface makes it easy to change >>> the vocabulary for priorities, resolutions, severities, and ticket types. I >>> also liked Stephan's workflow.
>>> I think this is a great addition to the core by the way.
>>> Daniel
>>> On Tue, Jul 13, 2010 at 4:01 PM, Mike Conley <mike.d.con...@gmail.com>wrote:
>>> > Ok, I'm fine with that. Unless there are further objections, Issue it >>> > is.
>>> > Hey David and Stephen - thank you both for the feedback and >>> > suggestions!
>>> > > 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.
>>> > Part of my task is to leave the Issue tracking/reporting open for >>> > extension so that you *can* have those dropdowns if you need them.
>>> > I'm working on a project with Review Board where we'll probably want >>> > the same type of dropdowns (specifically - issue type and severity). >>> > Once my work getting the bare-bones Issue business in, I'll build that >>> > extension, and then if you'd like, you can modify that extension to >>> > suit your purposes.
>>> > Because in the end, if we were to toss those dropdowns in, someone >>> > would undoubtably complain that they don't *exactly* fit their >>> > workflow, etc...and we'd be sunk. So I'm adopting a strategy of uber- >>> > simple + extensibility. Everybody (hopefully) wins! :D
>>> > Thanks everybody for all of your feedback, and for letting me think >>> > out loud. :D
>>> > -Mike
>>> > On Jul 12, 6:10 pm, David Trowbridge <trowb...@gmail.com> wrote: >>> > > My personal favorite is "Issue", but no matter what we call it, we're >>> > > going to get the "how do I get these issues into my bug tracker" >>> > > question, to which our answer is that "these aren't product bugs, >>> > > they're requested code changes that people want to track." As long as >>> > > we emphasize that it's a new workflow element instead of something >>> > > that they already have, I think we'll be fine.
>>> > > -David
>>> > > On Mon, Jul 12, 2010 at 4:23 AM, Stephen Gallagher
>>> > > <step...@gallagherhome.com> wrote: >>> > > > On 7/12/2010 12:55 AM, Mike Conley wrote:
>>> > > >> Ok, more cracks at it:
>>> > > >> Problem >>> > > >> Problem Solved >>> > > >> Not a Problem
>>> > > > 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.\
Hello to Poland. I visited your country last summer, and loved it.
Wroclaw is gorgeous this time of year. :D
So, just to reinforce what David is saying here, the defect/bug
tracking you mentioned, and the "issue tracking" feature I'm
suggesting are actually two separate things.
Defect/bug tracking is for problems that have already entered the code
base. A developer or a user has discovered a flaw in the software,
which has been tracked down to a bug in the code, and so we track that
bug until we can "squash" it.
The "issue tracking" feature I'm suggesting is *strictly* for review
requests - so, stuff that has either not entered the code base yet
(pre-commit code review), or has been committed (post-commit code
review) but hasn't been given the full green-light yet. The issue
tracking lets us keep track of the problems with the proposed
changes. Some easy examples: "Trim the whitespace"..."alphabetize
your imports here"... "invert the logic of this if/else clause", etc.
Does that make it any clearer?
> How integration between defect tracking tools and Your Review Board
> functionality looks like?
As far as I can tell, Review Board currently allows you to include the
defect/bug #'s from your bug tracker to a review request, in the event
that your review request addresses a particular defect/bug. Review
Board lets you easily link to your tracker, but I think the
interaction doesn't go much further.
HTH,
-Mike
On Jul 19, 3:38 am, Jan Koprowski <jan.koprow...@gmail.com> wrote:
> How integration between defect tracking tools and Your Review Board
> functionality looks like?
> On Mon, Jul 19, 2010 at 8:15 AM, David Trowbridge <trowb...@gmail.com> wrote:
> > As I said, this isn't about defect tracking for the *product*, which I
> > agree that there are many great tools for. This is about providing a
> > way to aggregate and track defects in the change -- it's often easy to
> > miss fixing one thing when you're trying to respond to dozens of
> > requested changes, and improving the workflow for this is long
> > overdue. Most other code review tools have such a workflow.
> > -David
> > On Sun, Jul 18, 2010 at 11:00 PM, Jan Koprowski <jan.koprow...@gmail.com> wrote:
> >> Hi,
> >> This idea is blind alley in my opinion. We have very good and many
> >> stable, grate defect tracking tools and we love it. If I undestand You
> >> try implement very basic defect tracking functionality on the top of
> >> Review Board. This remind me Smarty (PHP Template system) - if we have
> >> already many template system on the top of PHP lets implement PHP in
> >> template system :| Smarty in my opinion is a blind alley and worst
> >> architecture idea for template system and If I understand Your idea
> >> clearly You do something similar.
> >> Instead integration with external tracking tools will be good idea.
> >> Greetings from Poland!
> >> --
> >> Jan Koprowski
> >> On 14 Lip, 01:46, Daniel Swid <da.s...@gmail.com> wrote:
> >>> Regarding choice of vocabulary, I don't think you'll be able to satisfy
> >>> everyone. If anyone used Trac, the admin interface makes it easy to change
> >>> the vocabulary for priorities, resolutions, severities, and ticket types. I
> >>> also liked Stephan's workflow.
> >>> I think this is a great addition to the core by the way.
> >>> Daniel
> >>> On Tue, Jul 13, 2010 at 4:01 PM, Mike Conley <mike.d.con...@gmail.com>wrote:
> >>> > Ok, I'm fine with that. Unless there are further objections, Issue it
> >>> > is.
> >>> > Hey David and Stephen - thank you both for the feedback and
> >>> > suggestions!
> >>> > > 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.
> >>> > Part of my task is to leave the Issue tracking/reporting open for
> >>> > extension so that you *can* have those dropdowns if you need them.
> >>> > I'm working on a project with Review Board where we'll probably want
> >>> > the same type of dropdowns (specifically - issue type and severity).
> >>> > Once my work getting the bare-bones Issue business in, I'll build that
> >>> > extension, and then if you'd like, you can modify that extension to
> >>> > suit your purposes.
> >>> > Because in the end, if we were to toss those dropdowns in, someone
> >>> > would undoubtably complain that they don't *exactly* fit their
> >>> > workflow, etc...and we'd be sunk. So I'm adopting a strategy of uber-
> >>> > simple + extensibility. Everybody (hopefully) wins! :D
> >>> > Thanks everybody for all of your feedback, and for letting me think
> >>> > out loud. :D
> >>> > -Mike
> >>> > On Jul 12, 6:10 pm, David Trowbridge <trowb...@gmail.com> wrote:
> >>> > > My personal favorite is "Issue", but no matter what we call it, we're
> >>> > > going to get the "how do I get these issues into my bug tracker"
> >>> > > question, to which our answer is that "these aren't product bugs,
> >>> > > they're requested code changes that people want to track." As long as
> >>> > > we emphasize that it's a new workflow element instead of something
> >>> > > that they already have, I think we'll be fine.
> >>> > > -David
> >>> > > On Mon, Jul 12, 2010 at 4:23 AM, Stephen Gallagher
> >>> > > <step...@gallagherhome.com> wrote:
> >>> > > > On 7/12/2010 12:55 AM, Mike Conley wrote:
> >>> > > >> Ok, more cracks at it:
> >>> > > >> Problem
> >>> > > >> Problem Solved
> >>> > > >> Not a Problem
> >>> > > > 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.\
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!
On Tue, Jul 20, 2010 at 4:39 AM, Mike Conley <mike.d.con...@gmail.com> wrote: > Hey Jan!
> Hello to Poland. I visited your country last summer, and loved it. > Wroclaw is gorgeous this time of year. :D
> So, just to reinforce what David is saying here, the defect/bug > tracking you mentioned, and the "issue tracking" feature I'm > suggesting are actually two separate things.
> Defect/bug tracking is for problems that have already entered the code > base. A developer or a user has discovered a flaw in the software, > which has been tracked down to a bug in the code, and so we track that > bug until we can "squash" it.
> The "issue tracking" feature I'm suggesting is *strictly* for review > requests - so, stuff that has either not entered the code base yet > (pre-commit code review), or has been committed (post-commit code > review) but hasn't been given the full green-light yet. The issue > tracking lets us keep track of the problems with the proposed > changes. Some easy examples: "Trim the whitespace"..."alphabetize > your imports here"... "invert the logic of this if/else clause", etc.
> Does that make it any clearer?
>> How integration between defect tracking tools and Your Review Board >> functionality looks like?
> As far as I can tell, Review Board currently allows you to include the > defect/bug #'s from your bug tracker to a review request, in the event > that your review request addresses a particular defect/bug. Review > Board lets you easily link to your tracker, but I think the > interaction doesn't go much further.
> HTH,
> -Mike
> On Jul 19, 3:38 am, Jan Koprowski <jan.koprow...@gmail.com> wrote: >> How integration between defect tracking tools and Your Review Board >> functionality looks like?
>> On Mon, Jul 19, 2010 at 8:15 AM, David Trowbridge <trowb...@gmail.com> wrote: >> > As I said, this isn't about defect tracking for the *product*, which I >> > agree that there are many great tools for. This is about providing a >> > way to aggregate and track defects in the change -- it's often easy to >> > miss fixing one thing when you're trying to respond to dozens of >> > requested changes, and improving the workflow for this is long >> > overdue. Most other code review tools have such a workflow.
>> > -David
>> > On Sun, Jul 18, 2010 at 11:00 PM, Jan Koprowski <jan.koprow...@gmail.com> wrote: >> >> Hi,
>> >> This idea is blind alley in my opinion. We have very good and many >> >> stable, grate defect tracking tools and we love it. If I undestand You >> >> try implement very basic defect tracking functionality on the top of >> >> Review Board. This remind me Smarty (PHP Template system) - if we have >> >> already many template system on the top of PHP lets implement PHP in >> >> template system :| Smarty in my opinion is a blind alley and worst >> >> architecture idea for template system and If I understand Your idea >> >> clearly You do something similar. >> >> Instead integration with external tracking tools will be good idea.
>> >> Greetings from Poland! >> >> -- >> >> Jan Koprowski
>> >> On 14 Lip, 01:46, Daniel Swid <da.s...@gmail.com> wrote: >> >>> Regarding choice of vocabulary, I don't think you'll be able to satisfy >> >>> everyone. If anyone used Trac, the admin interface makes it easy to change >> >>> the vocabulary for priorities, resolutions, severities, and ticket types. I >> >>> also liked Stephan's workflow.
>> >>> I think this is a great addition to the core by the way.
>> >>> Daniel
>> >>> On Tue, Jul 13, 2010 at 4:01 PM, Mike Conley <mike.d.con...@gmail.com>wrote:
>> >>> > Ok, I'm fine with that. Unless there are further objections, Issue it >> >>> > is.
>> >>> > Hey David and Stephen - thank you both for the feedback and >> >>> > suggestions!
>> >>> > > 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.
>> >>> > Part of my task is to leave the Issue tracking/reporting open for >> >>> > extension so that you *can* have those dropdowns if you need them.
>> >>> > I'm working on a project with Review Board where we'll probably want >> >>> > the same type of dropdowns (specifically - issue type and severity). >> >>> > Once my work getting the bare-bones Issue business in, I'll build that >> >>> > extension, and then if you'd like, you can modify that extension to >> >>> > suit your purposes.
>> >>> > Because in the end, if we were to toss those dropdowns in, someone >> >>> > would undoubtably complain that they don't *exactly* fit their >> >>> > workflow, etc...and we'd be sunk. So I'm adopting a strategy of uber- >> >>> > simple + extensibility. Everybody (hopefully) wins! :D
>> >>> > Thanks everybody for all of your feedback, and for letting me think >> >>> > out loud. :D
>> >>> > -Mike
>> >>> > On Jul 12, 6:10 pm, David Trowbridge <trowb...@gmail.com> wrote: >> >>> > > My personal favorite is "Issue", but no matter what we call it, we're >> >>> > > going to get the "how do I get these issues into my bug tracker" >> >>> > > question, to which our answer is that "these aren't product bugs, >> >>> > > they're requested code changes that people want to track." As long as >> >>> > > we emphasize that it's a new workflow element instead of something >> >>> > > that they already have, I think we'll be fine.
>> >>> > > -David
>> >>> > > On Mon, Jul 12, 2010 at 4:23 AM, Stephen Gallagher
>> >>> > > <step...@gallagherhome.com> wrote: >> >>> > > > On 7/12/2010 12:55 AM, Mike Conley wrote:
>> >>> > > >> Ok, more cracks at it:
>> >>> > > >> Problem >> >>> > > >> Problem Solved >> >>> > > >> Not a Problem
>> >>> > > > 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.\
