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 <ceste...@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 <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 >> >
