I'm fine with all the above. Allowing for notes of justification to assist reviewers seems like a good way for us to avoid being pedantic about it. This is fairly subjective, after all.
On Wed, May 30, 2018 at 9:59 AM, Casey Stella <ceste...@gmail.com> wrote: > 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 >> >>> >> >> >> > >> >>
