Re: [openstack-dev] Criteria for giving a -1 in a review
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
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
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
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
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
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
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
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
-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
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
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
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
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
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
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
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
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
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
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
