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 18:20:51 GMT
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