Open Stack

On Mon, Apr 04, 2016 at 04:05:28PM +0300, Matthew Mosesohn wrote:
> I've seen several cases where core reviewers bully contributors into
> refactoring a particular piece of logic because it contains common
> lines relating to some non-ideal code, even if the change doesn't
> relate to this logic.
> In general, I'm ok with formatting issues, but changing how a piece of
> existing code works is over the line. It should be handled as a
> separate bug.

It's a judgement call, not a clear either-or. Core reviewers are people
who know better than others when particular code needs refactoring, and
they are more motivated than others to get it refactored, but if they
end up being the only ones ever doing refactoring, they end up
overwhelmed and the code rots.

So I think it's ok for core reviewers to enourage (although definitely
not to bully) other contributors to include well-isolated refactorings
with functional changes. The deciding factor shouldn't be whether the
changes are at all related to the bug in question, because this can and
will be taken ad absurdum and will encourage irresponsible patches that
quickly fix bugs by multiplying technical debt.

The deciding factor should be how much risk and how much additional
burden on reviewers would the requested refactoring add to the commit.
If it makes it easier to understand the affected code and doesn't have
functional impact outside of scope of the review, it's worth including
in the commit. If it has non-trivial functional impact, it can't really
be called a refactoring anyway, and in that case it does need a separate
bug or blueprint.

> But yes, in general, if someone complains about something unrelated to
> your patch, he or she should just file a bug with what is required.
> 
> -Matthew
> 
> 
> On Mon, Apr 4, 2016 at 3:46 PM, Dmitry Guryanov <dguryanov at mirantis.com> wrote:
> > Hello, colleagues!
> >
> > It's often not so easy to decide, if you should include some unrelated
> > changes to your patch, like fixing spaces, renaming variables or something
> > else, which don't change logic. On the one hand you see something's wrong
> > with the code and you'd like to fix it, on the other hand reviewers can vote
> > of -1 and you'll have to fix you patch and upload it again and this is very
> > annoying. You can also create separate review for such changes, but it will
> > require additional effort from you and reviewers.
> >
> > If you are a reviewer, and you've noted unrelated changes you may hesitate,
> > if you should ask an author to remove them and upload new version of the
> > patch or not. Also such extra changes may confuse you sometimes.
> >
> > So I suggest creating separate patches for unrelated changes if they add new
> > chucks to patch. And I'd like to ask authors to clearly state in the subject
> > of a commit message, that this patch just fixes formatting. And reviewers
> > shouldn't check such patches too severely, so that they'll get into repo as
> > soon as possible.
> >
> > What do you think?
> >
> >
> > --
> > Dmitry Guryanov
> >
> > __________________________________________________________________________
> > OpenStack Development Mailing List (not for usage questions)
> > Unsubscribe: OpenStack-dev-request at lists.openstack.org?subject:unsubscribe
> > http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
> >
> 
> __________________________________________________________________________
> OpenStack Development Mailing List (not for usage questions)
> Unsubscribe: OpenStack-dev-request at lists.openstack.org?subject:unsubscribe
> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev