I would appreciate if someone more conversant with sqoop1 can have some bullet points explaining what style checks it does. If not I can take a look at it when I get time.
Second, does that stop commits if code does not adhere to the style? Best, *./Vee* On Fri, Dec 5, 2014 at 12:13 PM, Jarek Jarcec Cecho <[email protected]> wrote: > If you run “ant checkstyle” in Sqoop 1 we will run a coding analysis to > see if it’s based on our coding guidelines. I believe that Venkat is > proposing to simply re-use that (albeit removing the limit for long lines). > > Jarcec > > > On Dec 5, 2014, at 10:20 AM, Veena Basavaraj <[email protected]> > wrote: > > > > Thanks for chiming in Venkat. Appreciate your time and inputs. > > > > What is the checkstyle we are referring to Venkat? can someone add some > > pointers to where is the guideline and how a developer can use it? Is > there > > a wiki ? or is it all in people's heads? > > > > Having said that, what guidelines we use less important than having a > > guideline which is completely lacking in sqoop2 at this point. > > > > > > > > > > > > > > Best, > > *./Vee* > > > > On Wed, Dec 3, 2014 at 2:35 PM, Abraham Elmahrek <[email protected]> > wrote: > > > >> 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. > >>> > >> > >
