Re: [openstack-dev] [fuel] Unrelated changes in patches

2016-04-04 Thread Dmitry Borodaenko 
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  
> 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-requ...@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-requ...@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-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [fuel] Unrelated changes in patches

2016-04-04 Thread Igor Kalnitsky 
Dmitry Guryanov wrote:
> 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.

I'd say it depends. If, for example, variable name is used inside one
function - it's ok to rename it within a patch. On the other hand, if
this variable is used across the code and it requires to change it in
few places - I'd prefer to do not do it within the patch. Any
unrelated change complicates review (if we're taking about thorough
review).

The things go worse when patch authors tries to implement two business
changes in one patch. In that case, it's really hard to distinguish
those both changes one from another in order to understand what's
going on.

So generally I'd prefer to see all unrelated changes in separate
patches. It's not necessary to create a bug for them, it's ok to
submit them with detailed commit message why this should be done.

Dmitry Guryanov wrote:
> 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 fix it in first patch, and make *business* changes in the second one.

Dmitry Guryanov wrote:
> You can also create separate review for such changes, but it will
> require additional effort from you and reviewers.

As reviewer I can say: I'm spending more time trying to figure out
what's going on in patch that changes two (or even more) unrelated
things, than I'd spend if I review those changes in independent
patches.

On Mon, Apr 4, 2016 at 3:46 PM, Dmitry Guryanov  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-requ...@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-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [fuel] Unrelated changes in patches

2016-04-04 Thread Jason Rist 
On 04/04/2016 07:05 AM, Matthew Mosesohn wrote:
> Hi Dmitry,
>
> 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.
>
> 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  
> 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-requ...@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-requ...@lists.openstack.org?subject:unsubscribe
> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
>
I agree with Matthew, but huge +1 to separate patch/bug for 
formatting/whitespace issues.

-- 
Jason E. Rist
Senior Software Engineer
OpenStack User Interfaces
Red Hat, Inc.
openuc: +1.972.707.6408
mobile: +1.720.256.3933
Freenode: jrist
github/twitter: knowncitizen

__
OpenStack Development Mailing List (not for usage questions)
Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [fuel] Unrelated changes in patches

2016-04-04 Thread Matthew Mosesohn 
Hi Dmitry,

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.

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  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-requ...@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-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

[openstack-dev] [fuel] Unrelated changes in patches

2016-04-04 Thread Dmitry Guryanov 
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-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev