Not to deviate much from the actual topic and instead focus on resolving this
I prefer #1 PS: formatting is not refactoring. If a import order changes because we have standardized on the rules it is really not code refactoring. If certain methods are renamed, duplicate code collapsed, yes I totally agree that such changes should be its own patch if possible. Best, *./Vee* On Tue, Dec 2, 2014 at 10:58 AM, Gwen Shapira <[email protected]> wrote: > I believe that the specific case where: > * patch has very few lines of actual change > * but has large number of imports > > Will be pretty rare and not worth a lot of discussion. I don't think > its productive to the discussion to focus on what is an edge case. > Lets try to agree on guidelines that most of us can live with most of the > time. > > In general, most of our patches are fairly large, and I think its a > good idea to keep our small patches small by not adding much extra > refactoring. > > > On Tue, Dec 2, 2014 at 8:18 AM, Veena Basavaraj <[email protected]> > wrote: > > Let me give a more concrete example > > > > #1, if we start using the standards uniformly from now, this means we > use a > > IDE formatter that adheres to these rules. > > > > So say I am modifying a existing file, I add some 5 new classes and needs > > imports, I usually tend to use the eclipse shortcut > > <http://www.rossenstoyanchev.org/write/prog/eclipse/eclipse3.html> > based on > > the formatter to do it, it will override and use the new order and there > > are bound to be changes. So according to you, we just append new imports > > manually to this file, so we dont create more than 3 lines of changes? > > > > I fail to understand why imports re-ordered makes it hard to read a > change > > even in git blame. It is very isolated change in the top of the file and > > can be easily ignored since we know we will be changing it going forward > to > > keep one standard. > > > > As annoying as it sounds to have this rule, I am happy to document this > > since everyone contributing to sqoop will be aware of this. > > > > > > > > > > > > Best, > > *./Vee* > > > > On Tue, Dec 2, 2014 at 7:57 AM, Gwen Shapira <[email protected]> > wrote: > > > >> #1 - yes. we had coding standards before, but they are documented now, > >> which makes for better overall experience for new contributors > >> > >> #2 - New code should obviously follow standards. > >> I support modifying existing code in separate JIRAs when the existing > >> code is particularly bad and a large change is required to make it > >> acceptable. > >> Otherwise, I like Jarcec suggestion: > >> - Cleanup can be done while working on unrelated patches, provided > >> that the scope of the cleanup is smaller than that of the patch. I.e. > >> - if the actual fix is 3 lines, don't add 100 lines of cleanup. If the > >> actual fix is 100 lines, 3 lines of low-risk cleanup are acceptable. > >> The idea is that when looking at the JIRA and the patch, 90% of the > >> patch should be related to the issue described in the JIRA. > >> > >> Gwen > >> > >> On Mon, Dec 1, 2014 at 7:04 PM, Veena Basavaraj < > [email protected]> > >> wrote: > >> > Very well articulated Gwen. > >> > > >> > So the question is > >> > #1. Should we start using code standards? > >> > #2, if we do, what is the best way to enforce it? > >> > > >> > > >> > > >> > > >> > > >> > Best, > >> > *./Vee* > >> > > >> > On Mon, Dec 1, 2014 at 6:39 PM, Gwen Shapira <[email protected]> > >> wrote: > >> > > >> >> Regarding applying the changes: > >> >> > >> >> I think we want to have good balance between few goals: > >> >> 1. Minimize risk of changes by having a well defined scope > >> >> 2. Make reviewing easier > >> >> 3. Ease of cherry-picking > >> >> 4. Ease of incrementally improving code quality > >> >> > >> >> Our current process leans toward the first three goals, but > >> >> discourages small improvements by adding quite a bit of process > around > >> >> them. > >> >> > >> >> I'd like to see us a bit more open toward small code improvements > that > >> >> do not add significant risk to the project. Spelling of variable > >> >> names, import order, indentation, comment readability, etc. I believe > >> >> it will lead to better code in the long term, without significant > >> >> drawbacks. > >> >> > >> >> Gwen > >> >> > >> >> > >> >> On Mon, Dec 1, 2014 at 6:17 PM, Jarek Jarcec Cecho < > [email protected]> > >> >> wrote: > >> >> > Thank you for putting the coding guidelines together Veena. I’ve > read > >> >> the page and I don’t see anything concerning. I have to admit that I > did > >> >> not read the whole Google guideline so I’m wondering if there are any > >> huge > >> >> differences between current (albeit unwritten) rules and the proposed > >> >> Google guidelines? > >> >> > > >> >> > I’ve added/edited the content a bit, trying to put some small > comments > >> >> without changing the semantics of the page. I have following larger > >> points: > >> >> > > >> >> > 1) Line size > >> >> > > >> >> > The “General Rules” section specifies that the “Column limit can > be 80 > >> >> or 100 characters”, the subsection “ Column Limit and Line Wrapping” > is > >> >> stating only “100” and then suggesting that we could also drop any > line > >> >> limits. I would suggest to synchronize those places together as it > seems > >> >> quite confusing. > >> >> > > >> >> > I would personally vote to not have any sharp limit for maximum > line > >> >> size. We have 80 characters limit in Sqoop 1 and it makes code very > >> hardly > >> >> readable at some places, so I like the Kafka wording “no maximum > size, > >> but > >> >> be reasonable”. > >> >> > > >> >> > 2) Applying the changes > >> >> > > >> >> > Considering that we might introduce some changes against current > >> >> (unwritten) rules, I think that we should talk about how we’re going > to > >> >> enforce the coding guidelines going forward. This is not a question > for > >> new > >> >> files as those should obviously follow them. I’m more interested to > >> discuss > >> >> how we’re going to handle changes in current files. As we are trying > to > >> >> keep each patch focusing on given functionality and we’re > discouraging > >> >> unrelated changes, I would propose to discourage people to enforce > those > >> >> rules in existing files if that would mean to significantly refactor > the > >> >> file just for the purpose of being compliant with our code > guidelines. > >> >> > > >> >> > This of course doesn’t mean that people should not follow the > >> guidelines > >> >> at all (new content should follow them) or that we’re going to stick > to > >> the > >> >> old formatting forever (on large file refactoring it would be OK to > >> apply > >> >> such changes). I’m just worried about introducing a lot of > unnecessary > >> >> changes just to keep in sync with coding guidelines. > >> >> > > >> >> > Jarcec > >> >> > > >> >> >> On Nov 30, 2014, at 6:29 PM, Veena Basavaraj < > >> [email protected]> > >> >> wrote: > >> >> >> > >> >> >> Qian, > >> >> >> > >> >> >> I think we both are on the same page. > >> >> >> > >> >> >> I just wrote the guidelines wiki to to come up with the code > style. > >> >> >> > >> >> >> One we all vote on the wiki, we can formulate those rules as IDE > >> >> specific > >> >> >> files, like the one you attached for intelliJ > >> >> >> > >> >> >> Hope it clears the confusion now. > >> >> >> > >> >> >> > >> >> >> > >> >> >> > >> >> >> Best, > >> >> >> *./Vee* > >> >> >> > >> >> >> On Sun, Nov 30, 2014 at 6:20 PM, Xu, Qian A <[email protected]> > >> >> wrote: > >> >> >> > >> >> >>> Hi Veena, > >> >> >>> > >> >> >>> A code guideline will definitively help both developers and > reviews > >> to > >> >> >>> contribute code better. I'd suggest apply code style to new code > >> >> >>> contribution first. > >> >> >>> > >> >> >>> --Qian (Stanley) Xu > >> >> >>> > >> >> >>> -----Original Message----- > >> >> >>> From: Veena Basavaraj [mailto:[email protected]] > >> >> >>> Sent: Monday, December 01, 2014 10:09 AM > >> >> >>> To: [email protected] > >> >> >>> Subject: Re: Sqoop 2 JAVA coding guidelines > >> >> >>> > >> >> >>> Thanks Qian, I will attach it, do you have any comments on the > >> >> guidelines? > >> >> >>> We should all vote for the wiki guidelines before we attach the > >> >> formatter. > >> >> >>> > >> >> >>> Please send an email to the dev@ list asking permission to edit > the > >> >> wiki. > >> >> >>> > >> >> >>> > >> >> >>> > >> >> >>> > >> >> >>> Best, > >> >> >>> *./Vee* > >> >> >>> > >> >> >>> On Sun, Nov 30, 2014 at 5:59 PM, Xu, Qian A <[email protected] > > > >> >> wrote: > >> >> >>> > >> >> >>>> Hi there. > >> >> >>>> > >> >> >>>> I don’t have the privilege to add attachment to Wiki , so I've > >> >> >>>> attached code formatter of IntelliJ (v13.1). I hope this helps. > >> >> >>>> > >> >> >>>> --Qian (Stanley) Xu > >> >> >>>> > >> >> >>>> > >> >> >>>> -----Original Message----- > >> >> >>>> From: Gwen Shapira [mailto:[email protected]] > >> >> >>>> Sent: Monday, December 01, 2014 8:46 AM > >> >> >>>> To: [email protected] > >> >> >>>> Subject: Re: Sqoop 2 JAVA coding guidelines > >> >> >>>> > >> >> >>>> I think its a great idea to document the coding standards that > >> already > >> >> >>>> exist for this project. > >> >> >>>> It can be frustrating to new contributors to have to guess their > >> way > >> >> >>>> around our un-written expectations. > >> >> >>>> > >> >> >>>> Gwen > >> >> >>>> > >> >> >>>> On Sun, Nov 30, 2014 at 2:05 PM, Veena Basavaraj > >> >> >>>> <[email protected]> > >> >> >>>> wrote: > >> >> >>>>> Hey all, > >> >> >>>>> > >> >> >>>>> Since a few months I have been contributing to Sqoop2, I do > >> notice a > >> >> >>>>> lack of style guide for sqoop2 code. > >> >> >>>>> > >> >> >>>>> I have started a wiki highlighting things for developers to > >> follow. > >> >> >>>>> > >> >> >>>>> > >> https://cwiki.apache.org/confluence/display/SQOOP/Sqoop+2+Coding+Gui > >> >> >>>>> de lines I prefer the google java style guide to begin with. > >> >> >>>>> > >> >> >>>>> Please add your comments in the comments section on things you > >> would > >> >> >>>>> like to have, so we can iterate over this in the coming few > weeks > >> >> >>>>> and settle down to a standard that every one can use in their > >> IDE. I > >> >> >>>>> am happy to create a formatter XML for eclipse as per these > rules > >> >> >>>>> and attach it to the wiki. > >> >> >>>>> > >> >> >>>>> > >> >> >>>>> > >> >> >>>>> Best, > >> >> >>>>> *./Vee* > >> >> >>>> > >> >> >>> > >> >> > > >> >> > >> >
