sqoop-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jarek Jarcec Cecho <jar...@apache.org>
Subject Re: Sqoop 2 JAVA coding guidelines
Date Fri, 05 Dec 2014 20:13:03 GMT
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 <vbasavaraj@cloudera.com> 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 <abe@cloudera.com> 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 <
>> vranganathan@hortonworks.com> 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 <jarcec@apache.org>
>>> 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 <
>> vbasavaraj@cloudera.com>
>>>> 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 <gshapira@cloudera.com
>>> 
>>>> 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 <
>>>> vbasavaraj@cloudera.com>
>>>>>> 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 <
>> gshapira@cloudera.com>
>>>>>> 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 <
>>>>>> vbasavaraj@cloudera.com>
>>>>>>>> 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 <
>>> gshapira@cloudera.com>
>>>>>>>> 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
<
>>>>>> jarcec@apache.org>
>>>>>>>>>> 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
<
>>>>>>>> vbasavaraj@cloudera.com>
>>>>>>>>>> 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 <
>>> qian.a.xu@intel.com>
>>>>>>>>>> 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:vbasavaraj@cloudera.com]
>>>>>>>>>>>>> Sent: Monday, December 01, 2014 10:09
AM
>>>>>>>>>>>>> To: dev@sqoop.apache.org
>>>>>>>>>>>>> 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 <
>>> qian.a.xu@intel.com
>>>>>>> 
>>>>>>>>>> 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:gshapira@cloudera.com]
>>>>>>>>>>>>>> Sent: Monday, December 01, 2014 8:46
AM
>>>>>>>>>>>>>> To: dev@sqoop.apache.org
>>>>>>>>>>>>>> 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
>>>>>>>>>>>>>> <vbasavaraj@cloudera.com>
>>>>>>>>>>>>>> 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.
>>> 
>> 


Mime
View raw message