sqoop-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Veena Basavaraj <vbasava...@cloudera.com>
Subject Re: Sqoop 2 JAVA coding guidelines
Date Fri, 05 Dec 2014 20:40:50 GMT
I would appreciate if someone more conversant with sqoop1 can have some
bullet points explaining what style checks it does. If not I can take a
look at it when I get time.

Second, does that stop commits if code does not adhere to the style?




Best,
*./Vee*

On Fri, Dec 5, 2014 at 12:13 PM, Jarek Jarcec Cecho <jarcec@apache.org>
wrote:

> 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
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message