Re: [openstack-dev] Criteria for giving a -1 in a review

2014-08-22 Thread Radomir Dopieralski
On 21/08/14 18:05, Matthew Booth wrote:

[snip]

 This seems to mean different things to different people. There's a list
 here which contains some criteria for new commits:

[snip]


 Any more of these?

There is also https://wiki.openstack.org/wiki/CodeReviewGuidelines

-- 
Radomir Dopieralski


___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev


Re: [openstack-dev] Criteria for giving a -1 in a review

2014-08-22 Thread Daniel P. Berrange
On Thu, Aug 21, 2014 at 04:52:37PM -0500, Dolph Mathews wrote:
 On Thu, Aug 21, 2014 at 11:53 AM, Daniel P. Berrange berra...@redhat.com
 wrote:
 
  On Thu, Aug 21, 2014 at 11:34:48AM -0500, Dolph Mathews wrote:
   On Thu, Aug 21, 2014 at 11:21 AM, Daniel P. Berrange 
  berra...@redhat.com
   wrote:
  
On Thu, Aug 21, 2014 at 05:05:04PM +0100, Matthew Booth wrote:
 I would prefer that you didn't merge this.

 i.e. The project is better off without it.
   
A bit off topic, but I've never liked this message that gets added
as it think it sounds overly negative. It would better written
as
   
  This patch needs further work before it can be merged
   
  
   ++ This patch needs further work before it can be merged, and as a
   reviewer, I am either too lazy or just unwilling to checkout your patch
  and
   fix those issues myself.
 
 
  I find the suggestion that reviewers are either too lazy or unwilling
  to fix it themselves rather distasteful to be honest.
 
 
 I should have followed the above with a gently sprinkling of /sarcasm and
 /dogfooding.

Ah, ok that explains it! The perils of communication via email :-)

[snip]

  I'd only recommend fixing  resubmitting someone else's patch if it is
  a really trivial thing that needed tweaking before approval for merge,
  or if they are known to be away for a prolonged time and the patch was
  blocking other important pending work.
 
 
 This is a great general rule. But with enough code reviews, there will be
 exceptions!

Of course. I'd always call these guidelines rather than rules since we
always want to retain the flexibility to ignore guidelines in exceptional
cases where they are counterproductive :-)

Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc :|

___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev


Re: [openstack-dev] Criteria for giving a -1 in a review

2014-08-22 Thread Steven Hardy
On Thu, Aug 21, 2014 at 05:05:04PM +0100, Matthew Booth wrote:
 I would prefer that you didn't merge this.
 
 i.e. The project is better off without it.

I'm not quite sure how you make that translation, I would interpret -2 as
meaning the project would be better off without a change.

FWIW, I've always interpreted I would prefer that you didn't merge this.
as having an implied suffix of (in it's current state).

 
 This seems to mean different things to different people. There's a list
 here which contains some criteria for new commits:
 
 https://wiki.openstack.org/wiki/ReviewChecklist.
 
 There's also a treatise on git commit messages and the structure of a
 commit here:
 
 https://wiki.openstack.org/wiki/GitCommitMessages
 
 However, these don't really cover the general case of what a -1 means.
 Here's my brain dump:
 
 * It contains bugs
 * It is likely to confuse future developers/maintainers
 * It is likely to lead to bugs
 * It is inconsistent with other solutions to similar problems
 * It adds complexity which is not matched by its benefits
 * It isn't flexible enough for future work landing RSN
 * It combines multiple changes in a single commit
 
 Any more? I'd be happy to update the above wiki page with any consensus.
 It would be useful if any generally accepted criteria were readily
 referenceable.
 
 I also think it's worth explicitly documenting a few things we
 might/should mention in a review, but which aren't a reason that the
 project would be better off without it:
 
 * Stylistic issues which are not covered by HACKING
 
 By stylistic, I mean changes which have no functional impact on the code
 whatsoever. If a purely stylistic issue is sufficiently important to
 reject code which doesn't adhere to it, it is important enough to add to
 HACKING.

I'll sometimes +1 a change if it looks functionally OK but has some
stylistic or cosmetic issues I would prefer to see fixed before giving a
+2.  I see that as a soft +2, it's not blocking anything, but I'm giving
the patch owner the chance to fix the problem (which they nearly always
do).

Although if a patch contains really significant uglies, I think giving a I
would prefer you didn't merge this, in it's current state with lots of
constructive comments wrt how to improve things is perfectly reasonable.

 * I can think of a better way of doing this
 
 There may be a better solution, but there is already an existing
 solution. We should only be rejecting work that has already been done if
 it would detract from the project for one of the reasons above. We can
 always improve it further later if we find the developer time.

Agreed, although again I'd encourage folks to +1 and leave detailed
information about how to improve the solution - most people (myself
included) really appreciate learning better ways to do things.  I've
definitely become a much better python developer as a result of the
detailed scrutiny and feedback provided via code reviews.

So while I agree with the general message you seem to be proposing (e.g
don't -1 for really trivial stuff), I think it's important to recognise
that if there are obvious and non-painful ways to improve code-quality, the
review is the time to do that.

I've been flamed before for saying this, but I maintain that part of the
reason we have so many (mostly new and non-core) reviewers leaving -1
feedback for really trivial stuff is that we collect, publish and in some
cases over-analyse review statistics.

Steve

___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev


Re: [openstack-dev] Criteria for giving a -1 in a review

2014-08-22 Thread Daniel P. Berrange
On Fri, Aug 22, 2014 at 10:49:51AM +0100, Steven Hardy wrote:
 On Thu, Aug 21, 2014 at 05:05:04PM +0100, Matthew Booth wrote:
  I also think it's worth explicitly documenting a few things we
  might/should mention in a review, but which aren't a reason that the
  project would be better off without it:
  
  * Stylistic issues which are not covered by HACKING
  
  By stylistic, I mean changes which have no functional impact on the code
  whatsoever. If a purely stylistic issue is sufficiently important to
  reject code which doesn't adhere to it, it is important enough to add to
  HACKING.
 
 I'll sometimes +1 a change if it looks functionally OK but has some
 stylistic or cosmetic issues I would prefer to see fixed before giving a
 +2.  I see that as a soft +2, it's not blocking anything, but I'm giving
 the patch owner the chance to fix the problem (which they nearly always
 do).
 
 Although if a patch contains really significant uglies, I think giving a I
 would prefer you didn't merge this, in it's current state with lots of
 constructive comments wrt how to improve things is perfectly reasonable.
 
  * I can think of a better way of doing this
  
  There may be a better solution, but there is already an existing
  solution. We should only be rejecting work that has already been done if
  it would detract from the project for one of the reasons above. We can
  always improve it further later if we find the developer time.
 
 Agreed, although again I'd encourage folks to +1 and leave detailed
 information about how to improve the solution - most people (myself
 included) really appreciate learning better ways to do things.  I've
 definitely become a much better python developer as a result of the
 detailed scrutiny and feedback provided via code reviews.
 
 So while I agree with the general message you seem to be proposing (e.g
 don't -1 for really trivial stuff), I think it's important to recognise
 that if there are obvious and non-painful ways to improve code-quality, the
 review is the time to do that.

One thing I have seen some people (eg Mark McLoughlin) do a number
of times is to actually submit followup patches. eg they will point
out the minor style issue, or idea for a better approach, but still
leave a +1/+2 score. Then submit a followup change to deal with that
nitpicking.  This seems like it is quite an effective approach
because it ensures the original authors' work gets through review
more quickly. Using a separate follow-on patch also avoids the idea
of the reviewer hijacking the original contributors patches by editing
them  reposting directly

Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc :|

___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev


[openstack-dev] Criteria for giving a -1 in a review

2014-08-21 Thread Matthew Booth
I would prefer that you didn't merge this.

i.e. The project is better off without it.

This seems to mean different things to different people. There's a list
here which contains some criteria for new commits:

https://wiki.openstack.org/wiki/ReviewChecklist.

There's also a treatise on git commit messages and the structure of a
commit here:

https://wiki.openstack.org/wiki/GitCommitMessages

However, these don't really cover the general case of what a -1 means.
Here's my brain dump:

* It contains bugs
* It is likely to confuse future developers/maintainers
* It is likely to lead to bugs
* It is inconsistent with other solutions to similar problems
* It adds complexity which is not matched by its benefits
* It isn't flexible enough for future work landing RSN
* It combines multiple changes in a single commit

Any more? I'd be happy to update the above wiki page with any consensus.
It would be useful if any generally accepted criteria were readily
referenceable.

I also think it's worth explicitly documenting a few things we
might/should mention in a review, but which aren't a reason that the
project would be better off without it:

* Stylistic issues which are not covered by HACKING

By stylistic, I mean changes which have no functional impact on the code
whatsoever. If a purely stylistic issue is sufficiently important to
reject code which doesn't adhere to it, it is important enough to add to
HACKING.

* I can think of a better way of doing this

There may be a better solution, but there is already an existing
solution. We should only be rejecting work that has already been done if
it would detract from the project for one of the reasons above. We can
always improve it further later if we find the developer time.

* It isn't flexible enough for any conceivable future feature

Lets avoid premature generalisation. We can always generalise as part of
landing the future feature.

Any more of these?

Thanks,

Matt
-- 
Matthew Booth
Red Hat Engineering, Virtualisation Team

Phone: +442070094448 (UK)
GPG ID:  D33C3490
GPG FPR: 3733 612D 2D05 5458 8A8A 1600 3441 EA19 D33C 3490

___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev


Re: [openstack-dev] Criteria for giving a -1 in a review

2014-08-21 Thread Daniel P. Berrange
On Thu, Aug 21, 2014 at 05:05:04PM +0100, Matthew Booth wrote:
 I would prefer that you didn't merge this.
 
 i.e. The project is better off without it.

A bit off topic, but I've never liked this message that gets added
as it think it sounds overly negative. It would better written
as

  This patch needs further work before it can be merged

as that gives a positive expectation that the work is still
wanted by the project in general


 This seems to mean different things to different people. There's a list
 here which contains some criteria for new commits:
 
 https://wiki.openstack.org/wiki/ReviewChecklist.
 
 There's also a treatise on git commit messages and the structure of a
 commit here:
 
 https://wiki.openstack.org/wiki/GitCommitMessages
 
 However, these don't really cover the general case of what a -1 means.
 Here's my brain dump:
 
 * It contains bugs
 * It is likely to confuse future developers/maintainers
 * It is likely to lead to bugs
 * It is inconsistent with other solutions to similar problems
 * It adds complexity which is not matched by its benefits
 * It isn't flexible enough for future work landing RSN

s/RSN//

There are times where the design is not flexible enough and we
do not want to accept regardless of when future work might land.
This is specifically the case with things that are adding APIs
or impacting the RPC protocol. For example proposals for new
virt driver methods that can't possibly work with other virt
drivers in the future and would involve incompatible RPC changes
to fix it.

 * It combines multiple changes in a single commit
 
 Any more? I'd be happy to update the above wiki page with any consensus.
 It would be useful if any generally accepted criteria were readily
 referenceable.

It is always worth improving our docs to give more guidance like
ths.

 I also think it's worth explicitly documenting a few things we
 might/should mention in a review, but which aren't a reason that the
 project would be better off without it:
 
 * Stylistic issues which are not covered by HACKING
 
 By stylistic, I mean changes which have no functional impact on the code
 whatsoever. If a purely stylistic issue is sufficiently important to
 reject code which doesn't adhere to it, it is important enough to add to
 HACKING.

Broadly speaking I agree with the idea that style cleanups should
have corresponding hacking rules validated automatically. If some
one proposes a style cleanup which isn't validated I'll typically
request that they write a hacking check to do so. There might be
some cases where it isn't practical to validate the rule automatically
which are none the less worthwhile -1'ing  for - these should be the
exception though. In general we shouldn't do style cleanups that we
can not automatically validate in some way.

 * I can think of a better way of doing this
 
 There may be a better solution, but there is already an existing
 solution. We should only be rejecting work that has already been done if
 it would detract from the project for one of the reasons above. We can
 always improve it further later if we find the developer time.
 
 * It isn't flexible enough for any conceivable future feature
 
 Lets avoid premature generalisation. We can always generalise as part of
 landing the future feature.

See my note about - it isn't always just about premature generalization
per se, but rather seeing things we are just clearly written from too
narrow a POV and will cause us pain down the line which could be easily
mitigated right away.

Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc :|

___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev


Re: [openstack-dev] Criteria for giving a -1 in a review

2014-08-21 Thread Dolph Mathews
On Thu, Aug 21, 2014 at 11:21 AM, Daniel P. Berrange berra...@redhat.com
wrote:

 On Thu, Aug 21, 2014 at 05:05:04PM +0100, Matthew Booth wrote:
  I would prefer that you didn't merge this.
 
  i.e. The project is better off without it.

 A bit off topic, but I've never liked this message that gets added
 as it think it sounds overly negative. It would better written
 as

   This patch needs further work before it can be merged


++ This patch needs further work before it can be merged, and as a
reviewer, I am either too lazy or just unwilling to checkout your patch and
fix those issues myself.

http://dolphm.com/reviewing-code


 as that gives a positive expectation that the work is still
 wanted by the project in general


  This seems to mean different things to different people. There's a list
  here which contains some criteria for new commits:
 
  https://wiki.openstack.org/wiki/ReviewChecklist.
 
  There's also a treatise on git commit messages and the structure of a
  commit here:
 
  https://wiki.openstack.org/wiki/GitCommitMessages
 
  However, these don't really cover the general case of what a -1 means.
  Here's my brain dump:
 
  * It contains bugs
  * It is likely to confuse future developers/maintainers
  * It is likely to lead to bugs
  * It is inconsistent with other solutions to similar problems
  * It adds complexity which is not matched by its benefits
  * It isn't flexible enough for future work landing RSN

 s/RSN//

 There are times where the design is not flexible enough and we
 do not want to accept regardless of when future work might land.
 This is specifically the case with things that are adding APIs
 or impacting the RPC protocol. For example proposals for new
 virt driver methods that can't possibly work with other virt
 drivers in the future and would involve incompatible RPC changes
 to fix it.

  * It combines multiple changes in a single commit
 
  Any more? I'd be happy to update the above wiki page with any consensus.
  It would be useful if any generally accepted criteria were readily
  referenceable.

 It is always worth improving our docs to give more guidance like
 ths.

  I also think it's worth explicitly documenting a few things we
  might/should mention in a review, but which aren't a reason that the
  project would be better off without it:
 
  * Stylistic issues which are not covered by HACKING
 
  By stylistic, I mean changes which have no functional impact on the code
  whatsoever. If a purely stylistic issue is sufficiently important to
  reject code which doesn't adhere to it, it is important enough to add to
  HACKING.

 Broadly speaking I agree with the idea that style cleanups should
 have corresponding hacking rules validated automatically. If some
 one proposes a style cleanup which isn't validated I'll typically
 request that they write a hacking check to do so. There might be
 some cases where it isn't practical to validate the rule automatically
 which are none the less worthwhile -1'ing  for - these should be the
 exception though. In general we shouldn't do style cleanups that we
 can not automatically validate in some way.

  * I can think of a better way of doing this
 
  There may be a better solution, but there is already an existing
  solution. We should only be rejecting work that has already been done if
  it would detract from the project for one of the reasons above. We can
  always improve it further later if we find the developer time.
 
  * It isn't flexible enough for any conceivable future feature
 
  Lets avoid premature generalisation. We can always generalise as part of
  landing the future feature.

 See my note about - it isn't always just about premature generalization
 per se, but rather seeing things we are just clearly written from too
 narrow a POV and will cause us pain down the line which could be easily
 mitigated right away.

 Regards,
 Daniel
 --
 |: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/
 :|
 |: http://libvirt.org  -o- http://virt-manager.org
 :|
 |: http://autobuild.org   -o- http://search.cpan.org/~danberr/
 :|
 |: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc
 :|

 ___
 OpenStack-dev mailing list
 OpenStack-dev@lists.openstack.org
 http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev


Re: [openstack-dev] Criteria for giving a -1 in a review

2014-08-21 Thread Adam Young

On 08/21/2014 12:21 PM, Daniel P. Berrange wrote:

On Thu, Aug 21, 2014 at 05:05:04PM +0100, Matthew Booth wrote:

I would prefer that you didn't merge this.

i.e. The project is better off without it.

A bit off topic, but I've never liked this message that gets added
as it think it sounds overly negative. It would better written
as

   This patch needs further work before it can be merged

Excellent.

It also bothers me that a -1 is dropped upon a new submission of the 
patch.  It seems to me that the review should instead indicate on a 
given line level comment whether it is grounds for -1.  If it is then 
either that same reviewer or another can decide whether a given fix 
address the reviewers request.



As a core reviewer, I have the power to -2 something.  That is 
considered a do not follow this approach message today.  I rarely 
exercise it, even for changes that I consider essential.  One reason is 
that a review with a -2 on it won't get additional reviews, and that is 
not my intention.







as that gives a positive expectation that the work is still
wanted by the project in general



This seems to mean different things to different people. There's a list
here which contains some criteria for new commits:

https://wiki.openstack.org/wiki/ReviewChecklist.

There's also a treatise on git commit messages and the structure of a
commit here:

https://wiki.openstack.org/wiki/GitCommitMessages

However, these don't really cover the general case of what a -1 means.
Here's my brain dump:

* It contains bugs
* It is likely to confuse future developers/maintainers
* It is likely to lead to bugs
* It is inconsistent with other solutions to similar problems
* It adds complexity which is not matched by its benefits
* It isn't flexible enough for future work landing RSN

s/RSN//

There are times where the design is not flexible enough and we
do not want to accept regardless of when future work might land.
This is specifically the case with things that are adding APIs
or impacting the RPC protocol. For example proposals for new
virt driver methods that can't possibly work with other virt
drivers in the future and would involve incompatible RPC changes
to fix it.


* It combines multiple changes in a single commit

Any more? I'd be happy to update the above wiki page with any consensus.
It would be useful if any generally accepted criteria were readily
referenceable.

It is always worth improving our docs to give more guidance like
ths.


I also think it's worth explicitly documenting a few things we
might/should mention in a review, but which aren't a reason that the
project would be better off without it:

* Stylistic issues which are not covered by HACKING

By stylistic, I mean changes which have no functional impact on the code
whatsoever. If a purely stylistic issue is sufficiently important to
reject code which doesn't adhere to it, it is important enough to add to
HACKING.

Broadly speaking I agree with the idea that style cleanups should
have corresponding hacking rules validated automatically. If some
one proposes a style cleanup which isn't validated I'll typically
request that they write a hacking check to do so. There might be
some cases where it isn't practical to validate the rule automatically
which are none the less worthwhile -1'ing  for - these should be the
exception though. In general we shouldn't do style cleanups that we
can not automatically validate in some way.


* I can think of a better way of doing this

There may be a better solution, but there is already an existing
solution. We should only be rejecting work that has already been done if
it would detract from the project for one of the reasons above. We can
always improve it further later if we find the developer time.

* It isn't flexible enough for any conceivable future feature

Lets avoid premature generalisation. We can always generalise as part of
landing the future feature.

See my note about - it isn't always just about premature generalization
per se, but rather seeing things we are just clearly written from too
narrow a POV and will cause us pain down the line which could be easily
mitigated right away.

Regards,
Daniel



___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev


Re: [openstack-dev] Criteria for giving a -1 in a review

2014-08-21 Thread Adam Young

On 08/21/2014 12:34 PM, Dolph Mathews wrote:


On Thu, Aug 21, 2014 at 11:21 AM, Daniel P. Berrange 
berra...@redhat.com mailto:berra...@redhat.com wrote:


On Thu, Aug 21, 2014 at 05:05:04PM +0100, Matthew Booth wrote:
 I would prefer that you didn't merge this.

 i.e. The project is better off without it.

A bit off topic, but I've never liked this message that gets added
as it think it sounds overly negative. It would better written
as

  This patch needs further work before it can be merged


++ This patch needs further work before it can be merged, and as a 
reviewer, I am either too lazy or just unwilling to checkout your 
patch and fix those issues myself.


Heh...well, there are a couple other aspects:

1.  I am unsure if my understanding is correct.  I'd like to have some 
validation, and, if I am wrong, I'll withdraw the objections.


2.  If I make the change, I can no longer +2/+A the review.  If you make 
the change, I can approve it.





http://dolphm.com/reviewing-code


as that gives a positive expectation that the work is still
wanted by the project in general


 This seems to mean different things to different people. There's
a list
 here which contains some criteria for new commits:

 https://wiki.openstack.org/wiki/ReviewChecklist.

 There's also a treatise on git commit messages and the structure
of a
 commit here:

 https://wiki.openstack.org/wiki/GitCommitMessages

 However, these don't really cover the general case of what a -1
means.
 Here's my brain dump:

 * It contains bugs
 * It is likely to confuse future developers/maintainers
 * It is likely to lead to bugs
 * It is inconsistent with other solutions to similar problems
 * It adds complexity which is not matched by its benefits
 * It isn't flexible enough for future work landing RSN

s/RSN//

There are times where the design is not flexible enough and we
do not want to accept regardless of when future work might land.
This is specifically the case with things that are adding APIs
or impacting the RPC protocol. For example proposals for new
virt driver methods that can't possibly work with other virt
drivers in the future and would involve incompatible RPC changes
to fix it.

 * It combines multiple changes in a single commit

 Any more? I'd be happy to update the above wiki page with any
consensus.
 It would be useful if any generally accepted criteria were readily
 referenceable.

It is always worth improving our docs to give more guidance like
ths.

 I also think it's worth explicitly documenting a few things we
 might/should mention in a review, but which aren't a reason that the
 project would be better off without it:

 * Stylistic issues which are not covered by HACKING

 By stylistic, I mean changes which have no functional impact on
the code
 whatsoever. If a purely stylistic issue is sufficiently important to
 reject code which doesn't adhere to it, it is important enough
to add to
 HACKING.

Broadly speaking I agree with the idea that style cleanups should
have corresponding hacking rules validated automatically. If some
one proposes a style cleanup which isn't validated I'll typically
request that they write a hacking check to do so. There might be
some cases where it isn't practical to validate the rule automatically
which are none the less worthwhile -1'ing  for - these should be the
exception though. In general we shouldn't do style cleanups that we
can not automatically validate in some way.

 * I can think of a better way of doing this

 There may be a better solution, but there is already an existing
 solution. We should only be rejecting work that has already been
done if
 it would detract from the project for one of the reasons above.
We can
 always improve it further later if we find the developer time.

 * It isn't flexible enough for any conceivable future feature

 Lets avoid premature generalisation. We can always generalise as
part of
 landing the future feature.

See my note about - it isn't always just about premature
generalization
per se, but rather seeing things we are just clearly written from too
narrow a POV and will cause us pain down the line which could be
easily
mitigated right away.

Regards,
Daniel
--
|: http://berrange.com -o-
http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org  -o- http://search.cpan.org/~danberr/
http://search.cpan.org/%7Edanberr/ :|
|: http://entangle-photo.org  -o- http://live.gnome.org/gtk-vnc :|

___
OpenStack-dev mailing list

Re: [openstack-dev] Criteria for giving a -1 in a review

2014-08-21 Thread Ihar Hrachyshka
-BEGIN PGP SIGNED MESSAGE-
Hash: SHA512

On 21/08/14 18:34, Dolph Mathews wrote:
 
 On Thu, Aug 21, 2014 at 11:21 AM, Daniel P. Berrange 
 berra...@redhat.com mailto:berra...@redhat.com wrote:
 
 On Thu, Aug 21, 2014 at 05:05:04PM +0100, Matthew Booth wrote:
 I would prefer that you didn't merge this.
 
 i.e. The project is better off without it.
 
 A bit off topic, but I've never liked this message that gets added 
 as it think it sounds overly negative. It would better written as
 
 This patch needs further work before it can be merged
 
 
 ++ This patch needs further work before it can be merged, and as
 a reviewer, I am either too lazy or just unwilling to checkout your
 patch and fix those issues myself.

Remember: in lots of cases, modifying patches from other people can be
considered offensive (you can't fix it in reasonable time) or
inconvenient (the guy may have local changes not sent for review yet,
and not he will need to checkout your version and meld them; and the
author may even forget it was modified by someone, so your changes may
be dropped during his next upload).

 
 http://dolphm.com/reviewing-code
 
 
 as that gives a positive expectation that the work is still wanted
 by the project in general
 
 
 This seems to mean different things to different people. There's
 a
 list
 here which contains some criteria for new commits:
 
 https://wiki.openstack.org/wiki/ReviewChecklist.
 
 There's also a treatise on git commit messages and the structure
 of a commit here:
 
 https://wiki.openstack.org/wiki/GitCommitMessages
 
 However, these don't really cover the general case of what a -1
 means. Here's my brain dump:
 
 * It contains bugs * It is likely to confuse future
 developers/maintainers * It is likely to lead to bugs * It is
 inconsistent with other solutions to similar problems * It adds
 complexity which is not matched by its benefits * It isn't
 flexible enough for future work landing RSN
 
 s/RSN//
 
 There are times where the design is not flexible enough and we do
 not want to accept regardless of when future work might land. This
 is specifically the case with things that are adding APIs or
 impacting the RPC protocol. For example proposals for new virt
 driver methods that can't possibly work with other virt drivers in
 the future and would involve incompatible RPC changes to fix it.
 
 * It combines multiple changes in a single commit
 
 Any more? I'd be happy to update the above wiki page with any
 consensus.
 It would be useful if any generally accepted criteria were
 readily referenceable.
 
 It is always worth improving our docs to give more guidance like 
 ths.
 
 I also think it's worth explicitly documenting a few things we 
 might/should mention in a review, but which aren't a reason that
 the project would be better off without it:
 
 * Stylistic issues which are not covered by HACKING
 
 By stylistic, I mean changes which have no functional impact on
 the code
 whatsoever. If a purely stylistic issue is sufficiently important
 to reject code which doesn't adhere to it, it is important enough
 to
 add to
 HACKING.
 
 Broadly speaking I agree with the idea that style cleanups should 
 have corresponding hacking rules validated automatically. If some 
 one proposes a style cleanup which isn't validated I'll typically 
 request that they write a hacking check to do so. There might be 
 some cases where it isn't practical to validate the rule
 automatically which are none the less worthwhile -1'ing  for -
 these should be the exception though. In general we shouldn't do
 style cleanups that we can not automatically validate in some way.
 
 * I can think of a better way of doing this
 
 There may be a better solution, but there is already an existing 
 solution. We should only be rejecting work that has already been
 done if
 it would detract from the project for one of the reasons above.
 We can always improve it further later if we find the developer
 time.
 
 * It isn't flexible enough for any conceivable future feature
 
 Lets avoid premature generalisation. We can always generalise as
 part of
 landing the future feature.
 
 See my note about - it isn't always just about premature
 generalization per se, but rather seeing things we are just clearly
 written from too narrow a POV and will cause us pain down the line
 which could be easily mitigated right away.
 
 Regards, Daniel -- |: http://berrange.com  -o- 
 http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org
 -o- http://virt-manager.org :| |: http://autobuild.org   -o-
  http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org
 -o- http://live.gnome.org/gtk-vnc :|
 
 ___ OpenStack-dev
 mailing list OpenStack-dev@lists.openstack.org 
 mailto:OpenStack-dev@lists.openstack.org 
 http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
 
 
 
 
 ___ OpenStack-dev
 mailing list 

Re: [openstack-dev] Criteria for giving a -1 in a review

2014-08-21 Thread Daniel P. Berrange
On Thu, Aug 21, 2014 at 11:34:48AM -0500, Dolph Mathews wrote:
 On Thu, Aug 21, 2014 at 11:21 AM, Daniel P. Berrange berra...@redhat.com
 wrote:
 
  On Thu, Aug 21, 2014 at 05:05:04PM +0100, Matthew Booth wrote:
   I would prefer that you didn't merge this.
  
   i.e. The project is better off without it.
 
  A bit off topic, but I've never liked this message that gets added
  as it think it sounds overly negative. It would better written
  as
 
This patch needs further work before it can be merged
 
 
 ++ This patch needs further work before it can be merged, and as a
 reviewer, I am either too lazy or just unwilling to checkout your patch and
 fix those issues myself.

I find the suggestion that reviewers are either too lazy or unwilling
to fix it themselves rather distasteful to be honest.

It is certainly valid for a code reviewer to fix an issue themselves 
re-post the patch, but that is not something to encourage a general day
to day practice for a number of reasons.

 - When there are multiple people reviewing it would quickly become a
   mess of conflicts if each  every reviewer took it upon themselves
   to rework  repost the patch.

 - The original submitter should generally always have the chance to
   rebut any feedback from reviewers, since reviewers are not infallible,
   nor always aware of the bigger picture or as familiar with the code
   being changed. 

 - When a patch is a small part of a larger series, it can be a very
   disruptive if someone else takes it, changes it  resubmits it,
   as that invalidates all following patches in a series in gerrit.

 - It does not scale to have reviewers take on much of the burden of
   actually writing the fixes, running the tests  resubmitting.

 - Having the original author deal with the review feedback actually
   helps that contributor learn new things, so that they will be able
   to do a better job for future patches they contribute

I'd only recommend fixing  resubmitting someone else's patch if it is
a really trivial thing that needed tweaking before approval for merge,
or if they are known to be away for a prolonged time and the patch was
blocking other important pending work.

Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc :|

___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev


Re: [openstack-dev] Criteria for giving a -1 in a review

2014-08-21 Thread Vishvananda Ishaya

On Aug 21, 2014, at 9:42 AM, Adam Young ayo...@redhat.com wrote:

 On 08/21/2014 12:34 PM, Dolph Mathews wrote:
 
 On Thu, Aug 21, 2014 at 11:21 AM, Daniel P. Berrange berra...@redhat.com 
 wrote:
 On Thu, Aug 21, 2014 at 05:05:04PM +0100, Matthew Booth wrote:
  I would prefer that you didn't merge this.
 
  i.e. The project is better off without it.
 
 A bit off topic, but I've never liked this message that gets added
 as it think it sounds overly negative. It would better written
 as
 
   This patch needs further work before it can be merged
 
 ++ This patch needs further work before it can be merged, and as a 
 reviewer, I am either too lazy or just unwilling to checkout your patch and 
 fix those issues myself.
 
 Heh...well, there are a couple other aspects:
 
 1.  I am unsure if my understanding is correct.  I'd like to have some 
 validation, and, if I am wrong, I'll withdraw the objections.
 
 2.  If I make the change, I can no longer +2/+A the review.  If you make the 
 change, I can approve it.

I don’t think this is correct. I’m totally ok with a core reviewer making a 
minor change to a patch AND +2ing it. This is especially true of minor things 
like spelling issues or code cleanliness. The only real functional difference 
between:

1) commenting “please change if foo==None: to if foo is None:”
2) waiting for the reviewer to exactly what you suggested
3) +2 the result

and:

1) you change if foo==None: to if foo is None: for the author
2) +2 the result

is the second set is WAY faster. Of course this only applies to minor changes. 
If you are refactoring more significantly it is nice to get the original 
poster’s feedback so the first option might be better.

Vish





signature.asc
Description: Message signed with OpenPGP using GPGMail
___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev


Re: [openstack-dev] Criteria for giving a -1 in a review

2014-08-21 Thread Daniel P. Berrange
On Thu, Aug 21, 2014 at 12:42:43PM -0400, Adam Young wrote:
 On 08/21/2014 12:34 PM, Dolph Mathews wrote:
 
 On Thu, Aug 21, 2014 at 11:21 AM, Daniel P. Berrange berra...@redhat.com
 mailto:berra...@redhat.com wrote:
 
 On Thu, Aug 21, 2014 at 05:05:04PM +0100, Matthew Booth wrote:
  I would prefer that you didn't merge this.
 
  i.e. The project is better off without it.
 
 A bit off topic, but I've never liked this message that gets added
 as it think it sounds overly negative. It would better written
 as
 
   This patch needs further work before it can be merged
 
 
 ++ This patch needs further work before it can be merged, and as a
 reviewer, I am either too lazy or just unwilling to checkout your patch
 and fix those issues myself.
 
 Heh...well, there are a couple other aspects:
 
 1.  I am unsure if my understanding is correct.  I'd like to have some
 validation, and, if I am wrong, I'll withdraw the objections.
 
 2.  If I make the change, I can no longer +2/+A the review.  If you make the
 change, I can approve it.

If it is something totally minor like a typo fix, or docs grammar
fix or whitespace cleanup it is reasonable to +2/+A something that
you took over from the original author, but that would be a pretty
rare scenario in general. Certainly a change which had any kind of
a functional impact, I'd not be happy with a person +2/+A'ing their
re-post.

Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc :|

___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev


Re: [openstack-dev] Criteria for giving a -1 in a review

2014-08-21 Thread Lance Bragstad
Comments inline below.


Best Regards,
Lance


On Thu, Aug 21, 2014 at 11:40 AM, Adam Young ayo...@redhat.com wrote:

 On 08/21/2014 12:21 PM, Daniel P. Berrange wrote:

 On Thu, Aug 21, 2014 at 05:05:04PM +0100, Matthew Booth wrote:

 I would prefer that you didn't merge this.

 i.e. The project is better off without it.

 A bit off topic, but I've never liked this message that gets added
 as it think it sounds overly negative. It would better written
 as

This patch needs further work before it can be merged

 Excellent.

 It also bothers me that a -1 is dropped upon a new submission of the
 patch.  It seems to me that the review should instead indicate on a given
 line level comment whether it is grounds for -1.  If it is then either that
 same reviewer or another can decide whether a given fix address the
 reviewers request.


 As a core reviewer, I have the power to -2 something.  That is considered
 a do not follow this approach message today.  I rarely exercise it, even
 for changes that I consider essential.  One reason is that a review with a
 -2 on it won't get additional reviews, and that is not my intention.



In this case, one way we can try to avoid the 'negativity' of a -2 is to
suggest the committer to mark the patch as Workflow -1 (WIP), and encourage
them to air out their explanation in project meeting open discussion and
IRC. To me, this changes the idea from this patch is going in a separate
direction than the project to this patch/idea hasn't been shot down, but
the committer needs a little help fleshing it out.





 as that gives a positive expectation that the work is still
 wanted by the project in general


  This seems to mean different things to different people. There's a list
 here which contains some criteria for new commits:

 https://wiki.openstack.org/wiki/ReviewChecklist.

 There's also a treatise on git commit messages and the structure of a
 commit here:

 https://wiki.openstack.org/wiki/GitCommitMessages

 However, these don't really cover the general case of what a -1 means.
 Here's my brain dump:

 * It contains bugs
 * It is likely to confuse future developers/maintainers
 * It is likely to lead to bugs
 * It is inconsistent with other solutions to similar problems
 * It adds complexity which is not matched by its benefits
 * It isn't flexible enough for future work landing RSN

 s/RSN//

 There are times where the design is not flexible enough and we
 do not want to accept regardless of when future work might land.
 This is specifically the case with things that are adding APIs
 or impacting the RPC protocol. For example proposals for new
 virt driver methods that can't possibly work with other virt
 drivers in the future and would involve incompatible RPC changes
 to fix it.

  * It combines multiple changes in a single commit

 Any more? I'd be happy to update the above wiki page with any consensus.
 It would be useful if any generally accepted criteria were readily
 referenceable.

 It is always worth improving our docs to give more guidance like
 ths.

  I also think it's worth explicitly documenting a few things we
 might/should mention in a review, but which aren't a reason that the
 project would be better off without it:

 * Stylistic issues which are not covered by HACKING

 By stylistic, I mean changes which have no functional impact on the code
 whatsoever. If a purely stylistic issue is sufficiently important to
 reject code which doesn't adhere to it, it is important enough to add to
 HACKING.

 Broadly speaking I agree with the idea that style cleanups should
 have corresponding hacking rules validated automatically. If some
 one proposes a style cleanup which isn't validated I'll typically
 request that they write a hacking check to do so. There might be
 some cases where it isn't practical to validate the rule automatically
 which are none the less worthwhile -1'ing  for - these should be the
 exception though. In general we shouldn't do style cleanups that we
 can not automatically validate in some way.

  * I can think of a better way of doing this

 There may be a better solution, but there is already an existing
 solution. We should only be rejecting work that has already been done if
 it would detract from the project for one of the reasons above. We can
 always improve it further later if we find the developer time.

 * It isn't flexible enough for any conceivable future feature

 Lets avoid premature generalisation. We can always generalise as part of
 landing the future feature.

 See my note about - it isn't always just about premature generalization
 per se, but rather seeing things we are just clearly written from too
 narrow a POV and will cause us pain down the line which could be easily
 mitigated right away.

 Regards,
 Daniel



 ___
 OpenStack-dev mailing list
 OpenStack-dev@lists.openstack.org
 http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev


Re: [openstack-dev] Criteria for giving a -1 in a review

2014-08-21 Thread Daniel P. Berrange
On Thu, Aug 21, 2014 at 12:40:59PM -0400, Adam Young wrote:
 On 08/21/2014 12:21 PM, Daniel P. Berrange wrote:
 On Thu, Aug 21, 2014 at 05:05:04PM +0100, Matthew Booth wrote:
 I would prefer that you didn't merge this.
 
 i.e. The project is better off without it.
 A bit off topic, but I've never liked this message that gets added
 as it think it sounds overly negative. It would better written
 as
 
This patch needs further work before it can be merged
 Excellent.
 
 It also bothers me that a -1 is dropped upon a new submission of the patch.
 It seems to me that the review should instead indicate on a given line level
 comment whether it is grounds for -1.  If it is then either that same
 reviewer or another can decide whether a given fix address the reviewers
 request.

I guess the idea of dropping the -1 is based on the understanding that
most contributors are working in good faith. ie that in the common case
they will actually address the review feedback before re-submitting a
new version. Sure some people violate this expectation, but in general
our contributor base does the right thing in this respect, which is good.

As a core reviewer I'd aim to look at the previous version to see if
there was an serious -1 there that was not address before approving
something anyway.

The problem with always preserving the -1 across re-posts, is that it
would discourage people from looking at new postings of the patch.
A gerrit query will show the -1's sitting there against the patch
with no indication that those -1s are probably stale and now
fixed by the new posting. So I really wouldn't want to see the -1's
preserved

Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc :|

___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev


Re: [openstack-dev] Criteria for giving a -1 in a review

2014-08-21 Thread Zane Bitter

On 21/08/14 12:21, Daniel P. Berrange wrote:

On Thu, Aug 21, 2014 at 05:05:04PM +0100, Matthew Booth wrote:

I would prefer that you didn't merge this.

i.e. The project is better off without it.

A bit off topic, but I've never liked this message that gets added
as it think it sounds overly negative. It would better written
as

   This patch needs further work before it can be merged

as that gives a positive expectation that the work is still
wanted by the project in general


Well, there are two audiences for that message: the developer and the 
reviewer. I can't help thinking that if instead of trying to be positive 
it said what it really means - Today, I have chosen to obstruct your 
work for the greater good of the project - we might have a few less -1s 
for trivial issues.


Maybe, while we're at it, we could stop publishing taxonomies of reasons 
to -1 a patch as if code reviews were a competition to see who can find 
the most.


cheers,
Zane.

___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev


Re: [openstack-dev] Criteria for giving a -1 in a review

2014-08-21 Thread Adam Young

On 08/21/2014 12:53 PM, Daniel P. Berrange wrote:

On Thu, Aug 21, 2014 at 11:34:48AM -0500, Dolph Mathews wrote:

On Thu, Aug 21, 2014 at 11:21 AM, Daniel P. Berrange berra...@redhat.com
wrote:


On Thu, Aug 21, 2014 at 05:05:04PM +0100, Matthew Booth wrote:

I would prefer that you didn't merge this.

i.e. The project is better off without it.

A bit off topic, but I've never liked this message that gets added
as it think it sounds overly negative. It would better written
as

   This patch needs further work before it can be merged


++ This patch needs further work before it can be merged, and as a
reviewer, I am either too lazy or just unwilling to checkout your patch and
fix those issues myself.

I find the suggestion that reviewers are either too lazy or unwilling
to fix it themselves rather distasteful to be honest.
That was from the Keystone PTL.  I think he was going for vaguely self 
deprecating  as opposed to dissing the reviewers.





It is certainly valid for a code reviewer to fix an issue themselves 
re-post the patch, but that is not something to encourage a general day
to day practice for a number of reasons.

  - When there are multiple people reviewing it would quickly become a
mess of conflicts if each  every reviewer took it upon themselves
to rework  repost the patch.

  - The original submitter should generally always have the chance to
rebut any feedback from reviewers, since reviewers are not infallible,
nor always aware of the bigger picture or as familiar with the code
being changed.

  - When a patch is a small part of a larger series, it can be a very
disruptive if someone else takes it, changes it  resubmits it,
as that invalidates all following patches in a series in gerrit.

  - It does not scale to have reviewers take on much of the burden of
actually writing the fixes, running the tests  resubmitting.

  - Having the original author deal with the review feedback actually
helps that contributor learn new things, so that they will be able
to do a better job for future patches they contribute

I'd only recommend fixing  resubmitting someone else's patch if it is
a really trivial thing that needed tweaking before approval for merge,
or if they are known to be away for a prolonged time and the patch was
blocking other important pending work.

Regards,
Daniel



___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev


Re: [openstack-dev] Criteria for giving a -1 in a review

2014-08-21 Thread Daniel P. Berrange
On Thu, Aug 21, 2014 at 01:12:16PM -0400, Zane Bitter wrote:
 On 21/08/14 12:21, Daniel P. Berrange wrote:
 On Thu, Aug 21, 2014 at 05:05:04PM +0100, Matthew Booth wrote:
 I would prefer that you didn't merge this.
 
 i.e. The project is better off without it.
 A bit off topic, but I've never liked this message that gets added
 as it think it sounds overly negative. It would better written
 as
 
This patch needs further work before it can be merged
 
 as that gives a positive expectation that the work is still
 wanted by the project in general
 
 Well, there are two audiences for that message: the developer and the
 reviewer. I can't help thinking that if instead of trying to be positive it
 said what it really means - Today, I have chosen to obstruct your work for
 the greater good of the project - we might have a few less -1s for trivial
 issues.
 
 Maybe, while we're at it, we could stop publishing taxonomies of reasons to
 -1 a patch as if code reviews were a competition to see who can find the
 most.

The reason for putting together examples of reasons to -1 a patch is not
to encourage people to -1 as much as possible. Rather the aim is to get
more consistency in how reviewers treat different types of problems. If
anything the intent of the mail is to actually help reviewers to allow
more trivial problems to get past review. Currently we give contributors
a inconsistent message with one reviewer saying some trivial thing should
be fixed while others will come along and say it is not worth fixing, or
can be fixed later on with a followup.

Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc :|

___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev


Re: [openstack-dev] Criteria for giving a -1 in a review

2014-08-21 Thread Dolph Mathews
On Thu, Aug 21, 2014 at 11:53 AM, Daniel P. Berrange berra...@redhat.com
wrote:

 On Thu, Aug 21, 2014 at 11:34:48AM -0500, Dolph Mathews wrote:
  On Thu, Aug 21, 2014 at 11:21 AM, Daniel P. Berrange 
 berra...@redhat.com
  wrote:
 
   On Thu, Aug 21, 2014 at 05:05:04PM +0100, Matthew Booth wrote:
I would prefer that you didn't merge this.
   
i.e. The project is better off without it.
  
   A bit off topic, but I've never liked this message that gets added
   as it think it sounds overly negative. It would better written
   as
  
 This patch needs further work before it can be merged
  
 
  ++ This patch needs further work before it can be merged, and as a
  reviewer, I am either too lazy or just unwilling to checkout your patch
 and
  fix those issues myself.


 I find the suggestion that reviewers are either too lazy or unwilling
 to fix it themselves rather distasteful to be honest.


I should have followed the above with a gently sprinkling of /sarcasm and
/dogfooding.



 It is certainly valid for a code reviewer to fix an issue themselves 
 re-post the patch, but that is not something to encourage a general day
 to day practice for a number of reasons.

  - When there are multiple people reviewing it would quickly become a
mess of conflicts if each  every reviewer took it upon themselves
to rework  repost the patch.


Valid theory, but I think you overestimate the amount of review bandwidth
the community actually has for this to be a problem :)



  - The original submitter should generally always have the chance to
rebut any feedback from reviewers, since reviewers are not infallible,
nor always aware of the bigger picture or as familiar with the code
being changed.


++ My comment is specifically with regard to nits. Real concerns should
always be discussed with the author.



  - When a patch is a small part of a larger series, it can be a very
disruptive if someone else takes it, changes it  resubmits it,
as that invalidates all following patches in a series in gerrit.


++ As a reviewer submitting a followup patchset, you certainly need to take
care of the entire series.



  - It does not scale to have reviewers take on much of the burden of
actually writing the fixes, running the tests  resubmitting.


Depends on what you're trying to scale for, I suppose. I'd like to see
valuable patches iterate and land more quickly, at the expense of lower
priority patches and my time as a reviewer.



  - Having the original author deal with the review feedback actually
helps that contributor learn new things, so that they will be able
to do a better job for future patches they contribute


+++ Even when a reviewer takes it upon themselves to contribute a patch,
I'd expect a normal -1 review explaining why things need to change, and
some sort of collaboration with the original author to make sure they're on
the same page.



 I'd only recommend fixing  resubmitting someone else's patch if it is
 a really trivial thing that needed tweaking before approval for merge,
 or if they are known to be away for a prolonged time and the patch was
 blocking other important pending work.


This is a great general rule. But with enough code reviews, there will be
exceptions!



 Regards,
 Daniel
 --
 |: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/
 :|
 |: http://libvirt.org  -o- http://virt-manager.org
 :|
 |: http://autobuild.org   -o- http://search.cpan.org/~danberr/
 :|
 |: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc
 :|

___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev


Re: [openstack-dev] Criteria for giving a -1 in a review

2014-08-21 Thread Tom Fifield
On 22/08/14 00:40, Adam Young wrote:
 On 08/21/2014 12:21 PM, Daniel P. Berrange wrote:
 On Thu, Aug 21, 2014 at 05:05:04PM +0100, Matthew Booth wrote:
 I would prefer that you didn't merge this.

 i.e. The project is better off without it.
 A bit off topic, but I've never liked this message that gets added
 as it think it sounds overly negative. It would better written
 as

This patch needs further work before it can be merged
 Excellent.


Clark has helpfully created a patch that would facilitate this change:

https://review.openstack.org/#/c/116176


Regards,


Tom

___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev