Open Stack

-----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 
> <berrange at redhat.com <mailto:berrange at 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 at lists.openstack.org 
> <mailto:OpenStack-dev at lists.openstack.org> 
> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
> 
> 
> 
> 
> _______________________________________________ OpenStack-dev
> mailing list OpenStack-dev at lists.openstack.org 
> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
> 
-----BEGIN PGP SIGNATURE-----
Version: GnuPG/MacGPG2 v2.0.22 (Darwin)

iQEcBAEBCgAGBQJT9iPCAAoJEC5aWaUY1u57AW0IAJASH/iwA/jSb0f0P9aaePU0
oB7bR1r+I2/91kSy/GWV3q21ujEmrlykEBLHtVjwSv8HvOwrInEc7TqKXoAlYztI
tq2RQvPhmZm8Y8qgTgOR7DPbWrTbSFsi7RMOok5eKEgr2D4MrzOJEahrGNPxpg2o
oqILjxzBMJy3VqWKry83R2cgLc5gSijdIriXN8DXuPY2vwMng80jMCe9jGWG3rMD
ALq/V3HpJ15oWfJn9wfRlLYX/cYFFapsK+oyINJ8tTrxyNU3cMQTPX799LZET8OK
t/xz2e7UBSfZ6nbkzGMzg8CLCWMHwTgk5PH/zz3uxsjEC9Am8YPPsSV0EU1RTTQ=
=KPcC
-----END PGP SIGNATURE-----