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. >>> >>
