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