I'm +1 on check style. I think there are even extensions with IDEs for checkstyle.
-Abe On Tue, Dec 2, 2014 at 8:57 PM, Venkat Ranganathan < [email protected]> wrote: > Why can't the guidelines we enforce with checkstyle in Sqoop 1 be > formalized and used with Sqoop2 (save for the line length restriction). > Is there any significant issues with that coding style? (Of course, we > have some errant checkins that break those also) > > Venkat > > On Tue, Dec 2, 2014 at 5:50 PM, Jarek Jarcec Cecho <[email protected]> > wrote: > > > Making formatting changes is also a refactoring from Git perspective - > you > > don’t have ability to “ignore” those when using git commands. > > > > I do see the point about not caring about ordering imports with git blame > > as I’m indeed not usually seeing why and when has been given import > > statement added - I’m looking into the actual code. However I have > > personally really bad experience when using git cherrypick on files with > a > > lot of import reorders and hence I do feel that the import re-orders > > belongs to the same category as anything else. If I’m significantly > > refactoring the file, sure let’s change the order. If I’m changing one > line > > in entire file, then I don’t necessarily see a need to make import > re-order > > change. > > > > Jarcec > > > > > On Dec 2, 2014, at 11:57 AM, Veena Basavaraj <[email protected]> > > wrote: > > > > > > 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* > > >>>>>>>>>> > > >>>>>>>>> > > >>>>>>> > > >>>>>> > > >>>> > > >> > > > > > > -- > CONFIDENTIALITY NOTICE > NOTICE: This message is intended for the use of the individual or entity to > which it is addressed and may contain information that is confidential, > privileged and exempt from disclosure under applicable law. If the reader > of this message is not the intended recipient, you are hereby notified that > any printing, copying, dissemination, distribution, disclosure or > forwarding of this communication is strictly prohibited. If you have > received this communication in error, please contact the sender immediately > and delete it from your system. Thank You. >
