metron-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Casey Stella <ceste...@gmail.com>
Subject Re: [DISCUSS] Refactoring
Date Wed, 30 May 2018 15:53:34 GMT
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 <cestella@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 <ottobackwards@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
>>>
>>
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message