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 Tue, 02 Dec 2014 19:57:32 GMT
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
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message