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:59:01 GMT
Yeah, that's true.

On Wed, May 30, 2018 at 8:58 AM Otto Fowler <ottobackwards@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 <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