hive-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Rui Li <lirui.fu...@gmail.com>
Subject Re: checkstyle changes
Date Fri, 08 Dec 2017 03:16:41 GMT
I also believe 140 is a little too long.

BTW do we use 2 or 4 chars for continuation indent? I personally prefer 4,
but I do find both cases in out code.

On Fri, Dec 8, 2017 at 6:20 AM, Alexander Kolbasov <akolb@cloudera.com>
wrote:

> Problem with 140-wide code isn't just laptops - in many cases we need to do
> side-by-side diffs (e.g. for code reviews) and this doubles the required
> size.
>
> - Alex.
>
> On Thu, Dec 7, 2017 at 1:38 PM, Sergey Shelukhin <sergey@hortonworks.com>
> wrote:
>
> > I think the 140-character change will make the code hard to use on a
> > laptop without a monitor.
> >
> >
> > On 17/12/7, 02:43, "Peter Vary" <pvary@cloudera.com> wrote:
> >
> > >Disclaimer: I did not have time to test it out, but according to
> > >http://checkstyle.sourceforge.net/config_misc.html#Indentation
> > ><http://checkstyle.sourceforge.net/config_misc.html#Indentation>
> > >Maybe the indentation could be solved by:
> > >lineWrappingIndentation=2 (default 4)
> > >forceStrictCondition=false (default false)
> > >
> > >http://checkstyle.sourceforge.net/config_misc.html#TrailingComment
> > ><http://checkstyle.sourceforge.net/config_misc.html#TrailingComment>
> > >might help with the comments
> > >
> > >Sorry for not being more helpful. Maybe sometime later I will have time
> > >to check these out.
> > >
> > >Thanks,
> > >Peter
> > >
> > >> On Dec 7, 2017, at 10:26 AM, Zoltan Haindrich
> > >><zhaindrich@hortonworks.com> wrote:
> > >>
> > >> Hello Eugene!
> > >>
> > >> I've looked into doing something with these; but I was not able to
> > >>relieve the warnings you've mentioned:
> > >>
> > >> * the ;// is seems to be not configurable
> > >>   It seems like its handled by the whitespaceafter module; I'm not
> sure
> > >>how to allow / after ;
> > >> * I think that indentation of 4 for many method arguments makes it
> more
> > >>readable; so I think it would be the best to just drop this check...but
> > >>I've not seen any way to do this(w/o disabling the whole indentation
> > >>module...)
> > >>
> > >> maybe someone else should take a look at it....I find it pretty hard
> to
> > >>get docs about specific chechkstyle configurations; since the search
> > >>keywords mostly contain keywords like: semicolon, whitespace,
> > >>comment...which tends to pull in all kind of garbage results :)
> > >>
> > >> cheers,
> > >> Zoltan
> > >>
> > >> On 6 Dec 2017 8:53 p.m., Eugene Koifman <ekoifman@hortonworks.com>
> > >>wrote:
> > >> It currently complains about no space between ; and // as in
> “…);//foo”
> > >>
> > >> And also about indentation when a single method call is split into
> > >>multiple lines.
> > >> It insists on 4 chars in this case, though we use 2 in (all?) other
> > >>cases.
> > >>
> > >> Could this be dialed down as well?
> > >>
> > >>
> > >> On 12/5/17, 7:26 AM, "Peter Vary" <pvary@cloudera.com> wrote:
> > >>
> > >>    +1 for the changes
> > >>
> > >>> On Dec 5, 2017, at 1:02 PM, Zoltan Haindrich <kirk@rxd.hu> wrote:
> > >>>
> > >>> Hello,
> > >>>
> > >>> I've filed a ticket to make the checkstyle warnings less noisy
> > >>>(https://issues.apache.org/jira/browse/HIVE-18222)
> > >>>
> > >>> * set maxlinelength to 140
> > >>>   I think everyone is working with big-enough displays to handle this
> > >>>:)
> > >>>   There are many methods which have complicated names / arguments /
> > >>>etc ; breaking the lines more frequently hurts readability...
> > >>> * disabled some restrictions like: declaration&hiding via get/set
> > >>>methods for protected/package fields are not mandatory
> > >>>
> > >>> If you don't feel comfortable with these changes, please share your
> > >>>point of view.
> > >>>
> > >>> cheers,
> > >>> Zoltan
> > >>>
> > >>>
> > >>
> > >>
> > >>
> > >>
> > >
> >
> >
>



-- 
Best regards!
Rui Li

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message