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