[DISCUSS] Refactoring

[Michael Miklavcic <mi...@gmail.com>]

I want to bring up the subject of code refactoring and how we should manage
this in PR's as our product evolves. As Metron matures, it's only natural
that we'll have and increasing number of contributors, and subsequently
contributions affecting many hardened parts of the code base. We've
generally not been particular about mixing refactoring changes with other
types of improvements or bug fixes. As a general best practice for software
engineering it is indeed desirable to undergo regular refactoring as a
matter of "scouts' rules" or "fixing broken windows." This helps keep code
readable and has the benefit of a fresh pair of eyes to see code in a new
way that allows the newcomer to introduce clarifying changes that the
original author(s) may not have considered.

While refactoring is generally applauded (because we have unit,
integration, and acceptance tests backing our changes), it does pose some
challenges during the review process. Depending on the type of PR, the
refactoring work can at times be many orders of magnitude larger than the
code pertinent to the desired change in functionality, whether bug fix or
feature enhancement, itself. While tests should protect against unintended
side effects (and sometimes they are also refactored) it does introduce the
possibility of new subtle bugs. It also makes a lot of PR's a conflated mix
of comments pertinent to the improvement/fix and opinions about best
practices around coding style.

I propose a simple change - we update our coding style guidelines in
section 2.1 to expand on refactoring. We currently cover whitespace and
comments:

"Don’t combine code changes with lots of edits of whitespace or comments;
it makes code review too difficult. It’s okay to fix an occasional comment
or indenting, but if wholesale comment or whitespace changes are needed,
make them a separate PR."

I propose we expand this to say:

"Don’t combine code changes with lots of edits of whitespace, comments, or
code changes specifically for refactoring purposes; it makes code review
too difficult. It’s okay to fix an occasional comment or indenting, but if
wholesale comment, whitespace or other refactoring changes are needed, make
them a separate PR."


I believe this provides additional clarity. I think it's one thing to
extract a method or introduce changes for code you're specifically
modifying, and another thing to introduce changes that affect surrounding
code. I would also propose we emphasize the Google checkstyle and
auto-formatting tooling when submitting any changes, but dealing with
enforcement is not my focus for this discuss thread.

https://cwiki.apache.org/confluence/display/METRON/Development+Guidelines

Best,
Michael Miklavcic

Re: [DISCUSS] Refactoring

[Michael Miklavcic <mi...@gmail.com>]

I'm fine with all the above. Allowing for notes of justification to assist
reviewers seems like a good way for us to avoid being pedantic about it.
This is fairly subjective, after all.

On Wed, May 30, 2018 at 9:59 AM, Casey Stella <ce...@gmail.com> wrote:

> Yeah, that's true.
>
> On Wed, May 30, 2018 at 8:58 AM Otto Fowler <ot...@gmail.com>
> wrote:
>
>> We can say that any refactoring that *is* necessary, needs to be written
>> out and justified in the review.
>> So, we don’t recommend it, but if you have to, and you can reasonably
>> defend it, OK.
>>
>>
>> On May 30, 2018 at 11:53:51, Casey Stella (cestella@gmail.com) wrote:
>>
>> Yep, I think we can, mike.
>>
>> Let me start with a emendation:
>>
>> "Don’t combine code changes with lots of edits of whitespace, comments,
>> or
>> code changes specifically for cosmetic refactoring purposes aimed solely
>> readability; it makes code review
>> and merging difficult. It’s okay to fix an occasional comment or
>> indentation, but if
>> wholesale comment, whitespace or other refactoring changes are needed,
>> make
>> them a separate PR."
>>
>>
>> On Wed, May 30, 2018 at 8:48 AM Michael Miklavcic <
>> michael.miklavcic@gmail.com> wrote:
>>
>> > Completely agreed on all points. Can we do that here and spin up a vote
>> > thread following with the final proposed changes?
>> >
>> > On Wed, May 30, 2018 at 9:46 AM, Casey Stella <ce...@gmail.com>
>> wrote:
>> >
>> >> I'm torn on this, honestly. I completely agree that cosmetic
>> refactoring
>> >> gets in the way of review and the risk can be more than the reward,
>> >> especially in a subtle bit of code.
>> >> That being said, I'm a big fan of opportunistically refactoring to
>> >> generalize or correct faulty assumptions. Often, I can't justify
>> making an
>> >> abstraction until I have seen the need more than once, so I will make
>> the
>> >> abstraction, as long as it's small and well-contained, in the PR
>> >> opportunistically, that motivates the 2nd usage. I like that kind of
>> >> opportunistic refactoring and I think that shouldn't be dissuaded.
>> >>
>> >> I agree with Otto, we should have a round of discussion on the doc
>> text
>> >> and I'd suggest we clarify to be cosmetic refactoring solely due to
>> >> readability concerns.
>> >>
>> >> Just my $0.02
>> >>
>> >> On Tue, May 29, 2018 at 7:40 PM Otto Fowler <ot...@gmail.com>
>> >> wrote:
>> >>
>> >>> On top of this, refactoring under another PR’s goals tends to be less
>> >>> documented as to the intent
>> >>> and effect.
>> >>>
>> >>> +1 for the idea, we should have a vote round or edit round on the
>> doc’s
>> >>> specific text.
>> >>> Although I will say, that some things it doesn’t matter how much you
>> >>> break
>> >>> them up wrt reviews.
>> >>> We should have so many reviewers that this is a problem.
>> >>>
>> >>>
>> >>>
>> >>>
>> >>> On May 29, 2018 at 20:05:49, Michael Miklavcic (
>> >>> michael.miklavcic@gmail.com)
>> >>> wrote:
>> >>>
>> >>> I want to bring up the subject of code refactoring and how we should
>> >>> manage
>> >>> this in PR's as our product evolves. As Metron matures, it's only
>> natural
>> >>> that we'll have and increasing number of contributors, and
>> subsequently
>> >>> contributions affecting many hardened parts of the code base. We've
>> >>> generally not been particular about mixing refactoring changes with
>> other
>> >>> types of improvements or bug fixes. As a general best practice for
>> >>> software
>> >>> engineering it is indeed desirable to undergo regular refactoring as
>> a
>> >>> matter of "scouts' rules" or "fixing broken windows." This helps keep
>> >>> code
>> >>> readable and has the benefit of a fresh pair of eyes to see code in a
>> new
>> >>> way that allows the newcomer to introduce clarifying changes that the
>> >>> original author(s) may not have considered.
>> >>>
>> >>> While refactoring is generally applauded (because we have unit,
>> >>> integration, and acceptance tests backing our changes), it does pose
>> some
>> >>> challenges during the review process. Depending on the type of PR,
>> the
>> >>> refactoring work can at times be many orders of magnitude larger than
>> the
>> >>> code pertinent to the desired change in functionality, whether bug
>> fix or
>> >>> feature enhancement, itself. While tests should protect against
>> >>> unintended
>> >>> side effects (and sometimes they are also refactored) it does
>> introduce
>> >>> the
>> >>> possibility of new subtle bugs. It also makes a lot of PR's a
>> conflated
>> >>> mix
>> >>> of comments pertinent to the improvement/fix and opinions about best
>> >>> practices around coding style.
>> >>>
>> >>> I propose a simple change - we update our coding style guidelines in
>> >>> section 2.1 to expand on refactoring. We currently cover whitespace
>> and
>> >>> comments:
>> >>>
>> >>> "Don’t combine code changes with lots of edits of whitespace or
>> comments;
>> >>> it makes code review too difficult. It’s okay to fix an occasional
>> >>> comment
>> >>> or indenting, but if wholesale comment or whitespace changes are
>> needed,
>> >>> make them a separate PR."
>> >>>
>> >>> I propose we expand this to say:
>> >>>
>> >>> "Don’t combine code changes with lots of edits of whitespace,
>> comments,
>> >>> or
>> >>> code changes specifically for refactoring purposes; it makes code
>> review
>> >>> too difficult. It’s okay to fix an occasional comment or indenting,
>> but
>> >>> if
>> >>> wholesale comment, whitespace or other refactoring changes are
>> needed,
>> >>> make
>> >>> them a separate PR."
>> >>>
>> >>>
>> >>> I believe this provides additional clarity. I think it's one thing to
>> >>> extract a method or introduce changes for code you're specifically
>> >>> modifying, and another thing to introduce changes that affect
>> surrounding
>> >>> code. I would also propose we emphasize the Google checkstyle and
>> >>> auto-formatting tooling when submitting any changes, but dealing with
>> >>> enforcement is not my focus for this discuss thread.
>> >>>
>> >>> https://cwiki.apache.org/confluence/display/METRON/
>> Development+Guidelines
>> >>>
>> >>> Best,
>> >>> Michael Miklavcic
>> >>>
>> >>
>> >
>>
>>

Re: [DISCUSS] Refactoring

[Casey Stella <ce...@gmail.com>]

Yeah, that's true.

On Wed, May 30, 2018 at 8:58 AM Otto Fowler <ot...@gmail.com> wrote:

> We can say that any refactoring that *is* necessary, needs to be written
> out and justified in the review.
> So, we don’t recommend it, but if you have to, and you can reasonably
> defend it, OK.
>
>
> On May 30, 2018 at 11:53:51, Casey Stella (cestella@gmail.com) wrote:
>
> Yep, I think we can, mike.
>
> Let me start with a emendation:
>
> "Don’t combine code changes with lots of edits of whitespace, comments, or
> code changes specifically for cosmetic refactoring purposes aimed solely
> readability; it makes code review
> and merging difficult. It’s okay to fix an occasional comment or
> indentation, but if
> wholesale comment, whitespace or other refactoring changes are needed,
> make
> them a separate PR."
>
>
> On Wed, May 30, 2018 at 8:48 AM Michael Miklavcic <
> michael.miklavcic@gmail.com> wrote:
>
> > Completely agreed on all points. Can we do that here and spin up a vote
> > thread following with the final proposed changes?
> >
> > On Wed, May 30, 2018 at 9:46 AM, Casey Stella <ce...@gmail.com>
> wrote:
> >
> >> I'm torn on this, honestly. I completely agree that cosmetic
> refactoring
> >> gets in the way of review and the risk can be more than the reward,
> >> especially in a subtle bit of code.
> >> That being said, I'm a big fan of opportunistically refactoring to
> >> generalize or correct faulty assumptions. Often, I can't justify making
> an
> >> abstraction until I have seen the need more than once, so I will make
> the
> >> abstraction, as long as it's small and well-contained, in the PR
> >> opportunistically, that motivates the 2nd usage. I like that kind of
> >> opportunistic refactoring and I think that shouldn't be dissuaded.
> >>
> >> I agree with Otto, we should have a round of discussion on the doc text
> >> and I'd suggest we clarify to be cosmetic refactoring solely due to
> >> readability concerns.
> >>
> >> Just my $0.02
> >>
> >> On Tue, May 29, 2018 at 7:40 PM Otto Fowler <ot...@gmail.com>
> >> wrote:
> >>
> >>> On top of this, refactoring under another PR’s goals tends to be less
> >>> documented as to the intent
> >>> and effect.
> >>>
> >>> +1 for the idea, we should have a vote round or edit round on the
> doc’s
> >>> specific text.
> >>> Although I will say, that some things it doesn’t matter how much you
> >>> break
> >>> them up wrt reviews.
> >>> We should have so many reviewers that this is a problem.
> >>>
> >>>
> >>>
> >>>
> >>> On May 29, 2018 at 20:05:49, Michael Miklavcic (
> >>> michael.miklavcic@gmail.com)
> >>> wrote:
> >>>
> >>> I want to bring up the subject of code refactoring and how we should
> >>> manage
> >>> this in PR's as our product evolves. As Metron matures, it's only
> natural
> >>> that we'll have and increasing number of contributors, and
> subsequently
> >>> contributions affecting many hardened parts of the code base. We've
> >>> generally not been particular about mixing refactoring changes with
> other
> >>> types of improvements or bug fixes. As a general best practice for
> >>> software
> >>> engineering it is indeed desirable to undergo regular refactoring as a
> >>> matter of "scouts' rules" or "fixing broken windows." This helps keep
> >>> code
> >>> readable and has the benefit of a fresh pair of eyes to see code in a
> new
> >>> way that allows the newcomer to introduce clarifying changes that the
> >>> original author(s) may not have considered.
> >>>
> >>> While refactoring is generally applauded (because we have unit,
> >>> integration, and acceptance tests backing our changes), it does pose
> some
> >>> challenges during the review process. Depending on the type of PR, the
> >>> refactoring work can at times be many orders of magnitude larger than
> the
> >>> code pertinent to the desired change in functionality, whether bug fix
> or
> >>> feature enhancement, itself. While tests should protect against
> >>> unintended
> >>> side effects (and sometimes they are also refactored) it does
> introduce
> >>> the
> >>> possibility of new subtle bugs. It also makes a lot of PR's a
> conflated
> >>> mix
> >>> of comments pertinent to the improvement/fix and opinions about best
> >>> practices around coding style.
> >>>
> >>> I propose a simple change - we update our coding style guidelines in
> >>> section 2.1 to expand on refactoring. We currently cover whitespace
> and
> >>> comments:
> >>>
> >>> "Don’t combine code changes with lots of edits of whitespace or
> comments;
> >>> it makes code review too difficult. It’s okay to fix an occasional
> >>> comment
> >>> or indenting, but if wholesale comment or whitespace changes are
> needed,
> >>> make them a separate PR."
> >>>
> >>> I propose we expand this to say:
> >>>
> >>> "Don’t combine code changes with lots of edits of whitespace,
> comments,
> >>> or
> >>> code changes specifically for refactoring purposes; it makes code
> review
> >>> too difficult. It’s okay to fix an occasional comment or indenting,
> but
> >>> if
> >>> wholesale comment, whitespace or other refactoring changes are needed,
> >>> make
> >>> them a separate PR."
> >>>
> >>>
> >>> I believe this provides additional clarity. I think it's one thing to
> >>> extract a method or introduce changes for code you're specifically
> >>> modifying, and another thing to introduce changes that affect
> surrounding
> >>> code. I would also propose we emphasize the Google checkstyle and
> >>> auto-formatting tooling when submitting any changes, but dealing with
> >>> enforcement is not my focus for this discuss thread.
> >>>
> >>>
> https://cwiki.apache.org/confluence/display/METRON/Development+Guidelines
> >>>
> >>> Best,
> >>> Michael Miklavcic
> >>>
> >>
> >
>
>

Re: [DISCUSS] Refactoring

[Otto Fowler <ot...@gmail.com>]

We can say that any refactoring that *is* necessary, needs to be written
out and justified in the review.
So, we don’t recommend it, but if you have to, and you can reasonably
defend it, OK.


On May 30, 2018 at 11:53:51, Casey Stella (cestella@gmail.com) wrote:

Yep, I think we can, mike.

Let me start with a emendation:

"Don’t combine code changes with lots of edits of whitespace, comments, or
code changes specifically for cosmetic refactoring purposes aimed solely
readability; it makes code review
and merging difficult. It’s okay to fix an occasional comment or
indentation, but if
wholesale comment, whitespace or other refactoring changes are needed, make
them a separate PR."


On Wed, May 30, 2018 at 8:48 AM Michael Miklavcic <
michael.miklavcic@gmail.com> wrote:

> Completely agreed on all points. Can we do that here and spin up a vote
> thread following with the final proposed changes?
>
> On Wed, May 30, 2018 at 9:46 AM, Casey Stella <ce...@gmail.com> wrote:
>
>> I'm torn on this, honestly. I completely agree that cosmetic refactoring
>> gets in the way of review and the risk can be more than the reward,
>> especially in a subtle bit of code.
>> That being said, I'm a big fan of opportunistically refactoring to
>> generalize or correct faulty assumptions. Often, I can't justify making
an
>> abstraction until I have seen the need more than once, so I will make
the
>> abstraction, as long as it's small and well-contained, in the PR
>> opportunistically, that motivates the 2nd usage. I like that kind of
>> opportunistic refactoring and I think that shouldn't be dissuaded.
>>
>> I agree with Otto, we should have a round of discussion on the doc text
>> and I'd suggest we clarify to be cosmetic refactoring solely due to
>> readability concerns.
>>
>> Just my $0.02
>>
>> On Tue, May 29, 2018 at 7:40 PM Otto Fowler <ot...@gmail.com>
>> wrote:
>>
>>> On top of this, refactoring under another PR’s goals tends to be less
>>> documented as to the intent
>>> and effect.
>>>
>>> +1 for the idea, we should have a vote round or edit round on the doc’s
>>> specific text.
>>> Although I will say, that some things it doesn’t matter how much you
>>> break
>>> them up wrt reviews.
>>> We should have so many reviewers that this is a problem.
>>>
>>>
>>>
>>>
>>> On May 29, 2018 at 20:05:49, Michael Miklavcic (
>>> michael.miklavcic@gmail.com)
>>> wrote:
>>>
>>> I want to bring up the subject of code refactoring and how we should
>>> manage
>>> this in PR's as our product evolves. As Metron matures, it's only
natural
>>> that we'll have and increasing number of contributors, and subsequently
>>> contributions affecting many hardened parts of the code base. We've
>>> generally not been particular about mixing refactoring changes with
other
>>> types of improvements or bug fixes. As a general best practice for
>>> software
>>> engineering it is indeed desirable to undergo regular refactoring as a
>>> matter of "scouts' rules" or "fixing broken windows." This helps keep
>>> code
>>> readable and has the benefit of a fresh pair of eyes to see code in a
new
>>> way that allows the newcomer to introduce clarifying changes that the
>>> original author(s) may not have considered.
>>>
>>> While refactoring is generally applauded (because we have unit,
>>> integration, and acceptance tests backing our changes), it does pose
some
>>> challenges during the review process. Depending on the type of PR, the
>>> refactoring work can at times be many orders of magnitude larger than
the
>>> code pertinent to the desired change in functionality, whether bug fix
or
>>> feature enhancement, itself. While tests should protect against
>>> unintended
>>> side effects (and sometimes they are also refactored) it does introduce
>>> the
>>> possibility of new subtle bugs. It also makes a lot of PR's a conflated
>>> mix
>>> of comments pertinent to the improvement/fix and opinions about best
>>> practices around coding style.
>>>
>>> I propose a simple change - we update our coding style guidelines in
>>> section 2.1 to expand on refactoring. We currently cover whitespace and
>>> comments:
>>>
>>> "Don’t combine code changes with lots of edits of whitespace or
comments;
>>> it makes code review too difficult. It’s okay to fix an occasional
>>> comment
>>> or indenting, but if wholesale comment or whitespace changes are
needed,
>>> make them a separate PR."
>>>
>>> I propose we expand this to say:
>>>
>>> "Don’t combine code changes with lots of edits of whitespace, comments,
>>> or
>>> code changes specifically for refactoring purposes; it makes code
review
>>> too difficult. It’s okay to fix an occasional comment or indenting, but
>>> if
>>> wholesale comment, whitespace or other refactoring changes are needed,
>>> make
>>> them a separate PR."
>>>
>>>
>>> I believe this provides additional clarity. I think it's one thing to
>>> extract a method or introduce changes for code you're specifically
>>> modifying, and another thing to introduce changes that affect
surrounding
>>> code. I would also propose we emphasize the Google checkstyle and
>>> auto-formatting tooling when submitting any changes, but dealing with
>>> enforcement is not my focus for this discuss thread.
>>>
>>>
https://cwiki.apache.org/confluence/display/METRON/Development+Guidelines
>>>
>>> Best,
>>> Michael Miklavcic
>>>
>>
>

Re: [DISCUSS] Refactoring

[Casey Stella <ce...@gmail.com>]

Yep, I think we can, mike.

Let me start with a emendation:

"Don’t combine code changes with lots of edits of whitespace, comments, or
code changes specifically for cosmetic refactoring purposes aimed solely
readability; it makes code review
and merging difficult. It’s okay to fix an occasional comment or
indentation, but if
wholesale comment, whitespace or other refactoring changes are needed, make
them a separate PR."


On Wed, May 30, 2018 at 8:48 AM Michael Miklavcic <
michael.miklavcic@gmail.com> wrote:

> Completely agreed on all points. Can we do that here and spin up a vote
> thread following with the final proposed changes?
>
> On Wed, May 30, 2018 at 9:46 AM, Casey Stella <ce...@gmail.com> wrote:
>
>> I'm torn on this, honestly.  I completely agree that cosmetic refactoring
>> gets in the way of review and the risk can be more than the reward,
>> especially in a subtle bit of code.
>> That being said, I'm a big fan of opportunistically refactoring to
>> generalize or correct faulty assumptions.  Often, I can't justify making an
>> abstraction until I have seen the need more than once, so I will make the
>> abstraction, as long as it's small and well-contained, in the PR
>> opportunistically, that motivates the 2nd usage.  I like that kind of
>> opportunistic refactoring and I think that shouldn't be dissuaded.
>>
>> I agree with Otto, we should have a round of discussion on the doc text
>> and I'd suggest we clarify to be cosmetic refactoring solely due to
>> readability concerns.
>>
>> Just my $0.02
>>
>> On Tue, May 29, 2018 at 7:40 PM Otto Fowler <ot...@gmail.com>
>> wrote:
>>
>>> On top of this, refactoring under another PR’s goals tends to be less
>>> documented as to the intent
>>> and effect.
>>>
>>> +1 for the idea, we should have a vote round or edit round on the doc’s
>>> specific text.
>>> Although I will say, that some things it doesn’t matter how much you
>>> break
>>> them up wrt reviews.
>>> We should have so many reviewers that this is a problem.
>>>
>>>
>>>
>>>
>>> On May 29, 2018 at 20:05:49, Michael Miklavcic (
>>> michael.miklavcic@gmail.com)
>>> wrote:
>>>
>>> I want to bring up the subject of code refactoring and how we should
>>> manage
>>> this in PR's as our product evolves. As Metron matures, it's only natural
>>> that we'll have and increasing number of contributors, and subsequently
>>> contributions affecting many hardened parts of the code base. We've
>>> generally not been particular about mixing refactoring changes with other
>>> types of improvements or bug fixes. As a general best practice for
>>> software
>>> engineering it is indeed desirable to undergo regular refactoring as a
>>> matter of "scouts' rules" or "fixing broken windows." This helps keep
>>> code
>>> readable and has the benefit of a fresh pair of eyes to see code in a new
>>> way that allows the newcomer to introduce clarifying changes that the
>>> original author(s) may not have considered.
>>>
>>> While refactoring is generally applauded (because we have unit,
>>> integration, and acceptance tests backing our changes), it does pose some
>>> challenges during the review process. Depending on the type of PR, the
>>> refactoring work can at times be many orders of magnitude larger than the
>>> code pertinent to the desired change in functionality, whether bug fix or
>>> feature enhancement, itself. While tests should protect against
>>> unintended
>>> side effects (and sometimes they are also refactored) it does introduce
>>> the
>>> possibility of new subtle bugs. It also makes a lot of PR's a conflated
>>> mix
>>> of comments pertinent to the improvement/fix and opinions about best
>>> practices around coding style.
>>>
>>> I propose a simple change - we update our coding style guidelines in
>>> section 2.1 to expand on refactoring. We currently cover whitespace and
>>> comments:
>>>
>>> "Don’t combine code changes with lots of edits of whitespace or comments;
>>> it makes code review too difficult. It’s okay to fix an occasional
>>> comment
>>> or indenting, but if wholesale comment or whitespace changes are needed,
>>> make them a separate PR."
>>>
>>> I propose we expand this to say:
>>>
>>> "Don’t combine code changes with lots of edits of whitespace, comments,
>>> or
>>> code changes specifically for refactoring purposes; it makes code review
>>> too difficult. It’s okay to fix an occasional comment or indenting, but
>>> if
>>> wholesale comment, whitespace or other refactoring changes are needed,
>>> make
>>> them a separate PR."
>>>
>>>
>>> I believe this provides additional clarity. I think it's one thing to
>>> extract a method or introduce changes for code you're specifically
>>> modifying, and another thing to introduce changes that affect surrounding
>>> code. I would also propose we emphasize the Google checkstyle and
>>> auto-formatting tooling when submitting any changes, but dealing with
>>> enforcement is not my focus for this discuss thread.
>>>
>>> https://cwiki.apache.org/confluence/display/METRON/Development+Guidelines
>>>
>>> Best,
>>> Michael Miklavcic
>>>
>>
>

Re: [DISCUSS] Refactoring

[Michael Miklavcic <mi...@gmail.com>]

Completely agreed on all points. Can we do that here and spin up a vote
thread following with the final proposed changes?

On Wed, May 30, 2018 at 9:46 AM, Casey Stella <ce...@gmail.com> wrote:

> I'm torn on this, honestly.  I completely agree that cosmetic refactoring
> gets in the way of review and the risk can be more than the reward,
> especially in a subtle bit of code.
> That being said, I'm a big fan of opportunistically refactoring to
> generalize or correct faulty assumptions.  Often, I can't justify making an
> abstraction until I have seen the need more than once, so I will make the
> abstraction, as long as it's small and well-contained, in the PR
> opportunistically, that motivates the 2nd usage.  I like that kind of
> opportunistic refactoring and I think that shouldn't be dissuaded.
>
> I agree with Otto, we should have a round of discussion on the doc text
> and I'd suggest we clarify to be cosmetic refactoring solely due to
> readability concerns.
>
> Just my $0.02
>
> On Tue, May 29, 2018 at 7:40 PM Otto Fowler <ot...@gmail.com>
> wrote:
>
>> On top of this, refactoring under another PR’s goals tends to be less
>> documented as to the intent
>> and effect.
>>
>> +1 for the idea, we should have a vote round or edit round on the doc’s
>> specific text.
>> Although I will say, that some things it doesn’t matter how much you break
>> them up wrt reviews.
>> We should have so many reviewers that this is a problem.
>>
>>
>>
>>
>> On May 29, 2018 at 20:05:49, Michael Miklavcic (
>> michael.miklavcic@gmail.com)
>> wrote:
>>
>> I want to bring up the subject of code refactoring and how we should
>> manage
>> this in PR's as our product evolves. As Metron matures, it's only natural
>> that we'll have and increasing number of contributors, and subsequently
>> contributions affecting many hardened parts of the code base. We've
>> generally not been particular about mixing refactoring changes with other
>> types of improvements or bug fixes. As a general best practice for
>> software
>> engineering it is indeed desirable to undergo regular refactoring as a
>> matter of "scouts' rules" or "fixing broken windows." This helps keep code
>> readable and has the benefit of a fresh pair of eyes to see code in a new
>> way that allows the newcomer to introduce clarifying changes that the
>> original author(s) may not have considered.
>>
>> While refactoring is generally applauded (because we have unit,
>> integration, and acceptance tests backing our changes), it does pose some
>> challenges during the review process. Depending on the type of PR, the
>> refactoring work can at times be many orders of magnitude larger than the
>> code pertinent to the desired change in functionality, whether bug fix or
>> feature enhancement, itself. While tests should protect against unintended
>> side effects (and sometimes they are also refactored) it does introduce
>> the
>> possibility of new subtle bugs. It also makes a lot of PR's a conflated
>> mix
>> of comments pertinent to the improvement/fix and opinions about best
>> practices around coding style.
>>
>> I propose a simple change - we update our coding style guidelines in
>> section 2.1 to expand on refactoring. We currently cover whitespace and
>> comments:
>>
>> "Don’t combine code changes with lots of edits of whitespace or comments;
>> it makes code review too difficult. It’s okay to fix an occasional comment
>> or indenting, but if wholesale comment or whitespace changes are needed,
>> make them a separate PR."
>>
>> I propose we expand this to say:
>>
>> "Don’t combine code changes with lots of edits of whitespace, comments, or
>> code changes specifically for refactoring purposes; it makes code review
>> too difficult. It’s okay to fix an occasional comment or indenting, but if
>> wholesale comment, whitespace or other refactoring changes are needed,
>> make
>> them a separate PR."
>>
>>
>> I believe this provides additional clarity. I think it's one thing to
>> extract a method or introduce changes for code you're specifically
>> modifying, and another thing to introduce changes that affect surrounding
>> code. I would also propose we emphasize the Google checkstyle and
>> auto-formatting tooling when submitting any changes, but dealing with
>> enforcement is not my focus for this discuss thread.
>>
>> https://cwiki.apache.org/confluence/display/METRON/Development+Guidelines
>>
>> Best,
>> Michael Miklavcic
>>
>

Re: [DISCUSS] Refactoring

[Casey Stella <ce...@gmail.com>]

I'm torn on this, honestly.  I completely agree that cosmetic refactoring
gets in the way of review and the risk can be more than the reward,
especially in a subtle bit of code.
That being said, I'm a big fan of opportunistically refactoring to
generalize or correct faulty assumptions.  Often, I can't justify making an
abstraction until I have seen the need more than once, so I will make the
abstraction, as long as it's small and well-contained, in the PR
opportunistically, that motivates the 2nd usage.  I like that kind of
opportunistic refactoring and I think that shouldn't be dissuaded.

I agree with Otto, we should have a round of discussion on the doc text and
I'd suggest we clarify to be cosmetic refactoring solely due to readability
concerns.

Just my $0.02

On Tue, May 29, 2018 at 7:40 PM Otto Fowler <ot...@gmail.com> wrote:

> On top of this, refactoring under another PR’s goals tends to be less
> documented as to the intent
> and effect.
>
> +1 for the idea, we should have a vote round or edit round on the doc’s
> specific text.
> Although I will say, that some things it doesn’t matter how much you break
> them up wrt reviews.
> We should have so many reviewers that this is a problem.
>
>
>
>
> On May 29, 2018 at 20:05:49, Michael Miklavcic (
> michael.miklavcic@gmail.com)
> wrote:
>
> I want to bring up the subject of code refactoring and how we should manage
> this in PR's as our product evolves. As Metron matures, it's only natural
> that we'll have and increasing number of contributors, and subsequently
> contributions affecting many hardened parts of the code base. We've
> generally not been particular about mixing refactoring changes with other
> types of improvements or bug fixes. As a general best practice for software
> engineering it is indeed desirable to undergo regular refactoring as a
> matter of "scouts' rules" or "fixing broken windows." This helps keep code
> readable and has the benefit of a fresh pair of eyes to see code in a new
> way that allows the newcomer to introduce clarifying changes that the
> original author(s) may not have considered.
>
> While refactoring is generally applauded (because we have unit,
> integration, and acceptance tests backing our changes), it does pose some
> challenges during the review process. Depending on the type of PR, the
> refactoring work can at times be many orders of magnitude larger than the
> code pertinent to the desired change in functionality, whether bug fix or
> feature enhancement, itself. While tests should protect against unintended
> side effects (and sometimes they are also refactored) it does introduce the
> possibility of new subtle bugs. It also makes a lot of PR's a conflated mix
> of comments pertinent to the improvement/fix and opinions about best
> practices around coding style.
>
> I propose a simple change - we update our coding style guidelines in
> section 2.1 to expand on refactoring. We currently cover whitespace and
> comments:
>
> "Don’t combine code changes with lots of edits of whitespace or comments;
> it makes code review too difficult. It’s okay to fix an occasional comment
> or indenting, but if wholesale comment or whitespace changes are needed,
> make them a separate PR."
>
> I propose we expand this to say:
>
> "Don’t combine code changes with lots of edits of whitespace, comments, or
> code changes specifically for refactoring purposes; it makes code review
> too difficult. It’s okay to fix an occasional comment or indenting, but if
> wholesale comment, whitespace or other refactoring changes are needed, make
> them a separate PR."
>
>
> I believe this provides additional clarity. I think it's one thing to
> extract a method or introduce changes for code you're specifically
> modifying, and another thing to introduce changes that affect surrounding
> code. I would also propose we emphasize the Google checkstyle and
> auto-formatting tooling when submitting any changes, but dealing with
> enforcement is not my focus for this discuss thread.
>
> https://cwiki.apache.org/confluence/display/METRON/Development+Guidelines
>
> Best,
> Michael Miklavcic
>

Re: [DISCUSS] Refactoring

[Otto Fowler <ot...@gmail.com>]

On top of this, refactoring under another PR’s goals tends to be less
documented as to the intent
and effect.

+1 for the idea, we should have a vote round or edit round on the doc’s
specific text.
Although I will say, that some things it doesn’t matter how much you break
them up wrt reviews.
We should have so many reviewers that this is a problem.




On May 29, 2018 at 20:05:49, Michael Miklavcic (michael.miklavcic@gmail.com)
wrote:

I want to bring up the subject of code refactoring and how we should manage
this in PR's as our product evolves. As Metron matures, it's only natural
that we'll have and increasing number of contributors, and subsequently
contributions affecting many hardened parts of the code base. We've
generally not been particular about mixing refactoring changes with other
types of improvements or bug fixes. As a general best practice for software
engineering it is indeed desirable to undergo regular refactoring as a
matter of "scouts' rules" or "fixing broken windows." This helps keep code
readable and has the benefit of a fresh pair of eyes to see code in a new
way that allows the newcomer to introduce clarifying changes that the
original author(s) may not have considered.

While refactoring is generally applauded (because we have unit,
integration, and acceptance tests backing our changes), it does pose some
challenges during the review process. Depending on the type of PR, the
refactoring work can at times be many orders of magnitude larger than the
code pertinent to the desired change in functionality, whether bug fix or
feature enhancement, itself. While tests should protect against unintended
side effects (and sometimes they are also refactored) it does introduce the
possibility of new subtle bugs. It also makes a lot of PR's a conflated mix
of comments pertinent to the improvement/fix and opinions about best
practices around coding style.

I propose a simple change - we update our coding style guidelines in
section 2.1 to expand on refactoring. We currently cover whitespace and
comments:

"Don’t combine code changes with lots of edits of whitespace or comments;
it makes code review too difficult. It’s okay to fix an occasional comment
or indenting, but if wholesale comment or whitespace changes are needed,
make them a separate PR."

I propose we expand this to say:

"Don’t combine code changes with lots of edits of whitespace, comments, or
code changes specifically for refactoring purposes; it makes code review
too difficult. It’s okay to fix an occasional comment or indenting, but if
wholesale comment, whitespace or other refactoring changes are needed, make
them a separate PR."


I believe this provides additional clarity. I think it's one thing to
extract a method or introduce changes for code you're specifically
modifying, and another thing to introduce changes that affect surrounding
code. I would also propose we emphasize the Google checkstyle and
auto-formatting tooling when submitting any changes, but dealing with
enforcement is not my focus for this discuss thread.

https://cwiki.apache.org/confluence/display/METRON/Development+Guidelines

Best,
Michael Miklavcic