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 <ottobackwa...@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.miklav...@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 >
