Yeah, that's true. On Wed, May 30, 2018 at 8:58 AM Otto Fowler <ottobackwa...@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 (ceste...@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.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 > >>> > >> > > > >
