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 Wed, 03 Dec 2014 01:50:03 GMT
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*
>>>>>>>>>> 
>>>>>>>>> 
>>>>>>> 
>>>>>> 
>>>> 
>> 


Mime
View raw message