sqoop-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Venkat Ranganathan <vranganat...@hortonworks.com>
Subject Re: Sqoop 2 JAVA coding guidelines
Date Wed, 03 Dec 2014 04:57:48 GMT
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
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message