flink-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Henry Saputra <henry.sapu...@gmail.com>
Subject Re: [DISCUSS] Java code style
Date Sat, 24 Oct 2015 18:33:28 GMT
+1 for adding restriction for Javadoc at least at the header of public
classes and methods.

We did the exercise in Twill and seemed to work pretty well.

On Fri, Oct 23, 2015 at 1:34 AM, Maximilian Michels <mxm@apache.org> wrote:
> I don't think lazily adding comments will work. However, I'm fine with
> adding all the checkstyle rules one module at a time (with a jira
> issue to keep track of the modules already converted). It's not going
> to happen that we lazily add comments because that's the reason why
> comments are missing in the first place...
>
> On Fri, Oct 23, 2015 at 12:05 AM, Henry Saputra <henry.saputra@gmail.com> wrote:
>> Could we make certain rules to give warning instead of error?
>>
>> This would allow us to cherry-pick certain rules we would like people
>> to follow but not strictly enforced.
>>
>> - Henry
>>
>> On Thu, Oct 22, 2015 at 9:20 AM, Stephan Ewen <sewen@apache.org> wrote:
>>> I don't think a "let add comments to everything" effort gives us good
>>> comments, actually. It just gives us checkmark comments that make the rules
>>> pass.
>>>
>>> On Thu, Oct 22, 2015 at 3:29 PM, Fabian Hueske <fhueske@gmail.com> wrote:
>>>
>>>> Sure, I don't expect it to be free.
>>>> But everybody should be aware of the cost of adding this code style, i.e.,
>>>> spending a huge amount of time on reformatting and documenting code.
>>>>
>>>> Alternatively, we could drop the JavaDocs rule and make the transition
>>>> significantly cheaper.
>>>>
>>>> 2015-10-22 15:24 GMT+02:00 Till Rohrmann <trohrmann@apache.org>:
>>>>
>>>> > There ain’t no such thing as a free lunch and code style.
>>>> >
>>>> > On Thu, Oct 22, 2015 at 3:13 PM, Maximilian Michels <mxm@apache.org>
>>>> > wrote:
>>>> >
>>>> > > I think we have to document all these classes. Code Style doesn't
come
>>>> > > for free :)
>>>> > >
>>>> > > On Thu, Oct 22, 2015 at 3:09 PM, Fabian Hueske <fhueske@gmail.com>
>>>> > wrote:
>>>> > > > Any ideas how to deal with the mandatory JavaDoc rule for
existing
>>>> > code?
>>>> > > > Just adding empty headers to make the checkstyle pass or start
a
>>>> > serious
>>>> > > > effort to add the missing docs?
>>>> > > >
>>>> > > > 2015-10-21 13:31 GMT+02:00 Matthias J. Sax <mjsax@apache.org>:
>>>> > > >
>>>> > > >> Agreed. That's the reason why I am in favor of using vanilla
Google
>>>> > code
>>>> > > >> style.
>>>> > > >>
>>>> > > >> On 10/21/2015 12:31 PM, Stephan Ewen wrote:
>>>> > > >> > We started out originally with mixed tab/spaces,
but it ended up
>>>> > with
>>>> > > >> > people mixing spaces and tabs arbitrarily, and there
is little way
>>>> > to
>>>> > > >> > enforce Matthias' specific suggestion via checkstyle.
>>>> > > >> > That's why we dropped spaces alltogether...
>>>> > > >> >
>>>> > > >> > On Wed, Oct 21, 2015 at 12:03 PM, Gyula Fóra <
>>>> gyula.fora@gmail.com>
>>>> > > >> wrote:
>>>> > > >> >
>>>> > > >> >> I think the nice thing about a common codestyle
is that everyone
>>>> > can
>>>> > > set
>>>> > > >> >> the template in the IDE and use the formatting
commands.
>>>> > > >> >>
>>>> > > >> >> Matthias's suggestion makes this practically
impossible so -1 for
>>>> > > mixed
>>>> > > >> >> tabs/spaces from my side.
>>>> > > >> >>
>>>> > > >> >> Matthias J. Sax <mjsax@apache.org> ezt
írta (időpont: 2015. okt.
>>>> > > 21.,
>>>> > > >> Sze,
>>>> > > >> >> 11:46):
>>>> > > >> >>
>>>> > > >> >>> I actually like tabs a lot, however, in a
"mixed" style together
>>>> > > with
>>>> > > >> >>> spaces. Example:
>>>> > > >> >>>
>>>> > > >> >>>         myVar.callMethod(param1, // many
more
>>>> > > >> >>>         .................paramX); // the
dots mark space
>>>> indention
>>>> > > >> >>>
>>>> > > >> >>> indenting "paramX" with tabs does not give
nice aliment. Not
>>>> sure
>>>> > if
>>>> > > >> >>> this would be a feasible compromise to keeps
tabs in general,
>>>> but
>>>> > > use
>>>> > > >> >>> space for cases as above.
>>>> > > >> >>>
>>>> > > >> >>> If this in no feasible compromise, I would
prefer space to get
>>>> the
>>>> > > >> >>> correct indention in examples as above. Even
if this result in a
>>>> > > >> >>> complete reformatting of the whole code.
>>>> > > >> >>>
>>>> > > >> >>>
>>>> > > >> >>> Why this? Everybody can set this in it's
IDE/editor as he/she
>>>> > > wishes...
>>>> > > >> >>>
>>>> > > >> >>>>> If we keep tabs, we will have to
specify the line length
>>>> > relative
>>>> > > to
>>>> > > >> a
>>>> > > >> >>> tab
>>>> > > >> >>>>> size (like 4).
>>>> > > >> >>>
>>>> > > >> >>>
>>>> > > >> >>> -Matthias
>>>> > > >> >>>
>>>> > > >> >>>
>>>> > > >> >>>
>>>> > > >> >>>
>>>> > > >> >>>
>>>> > > >> >>> On 10/21/2015 11:06 AM, Ufuk Celebi wrote:
>>>> > > >> >>>> To summarize up to this point:
>>>> > > >> >>>>
>>>> > > >> >>>> - All are in favour of Google check style
(with the following
>>>> > > possible
>>>> > > >> >>>> exceptions)
>>>> > > >> >>>> - Proposed exceptions so far:
>>>> > > >> >>>>   * Specific line length 100 vs. 120
characters
>>>> > > >> >>>>   * Keep tabs instead converting to spaces
(this would
>>>> translate
>>>> > to
>>>> > > >> >>>> skipping/coming up with some indentation
rules as well)
>>>> > > >> >>>>
>>>> > > >> >>>> If we keep tabs, we will have to specify
the line length
>>>> relative
>>>> > > to a
>>>> > > >> >>> tab
>>>> > > >> >>>> size (like 4).
>>>> > > >> >>>>
>>>> > > >> >>>> Let’s keep the discussion going a little
longer. I think it has
>>>> > > >> >> proceeded
>>>> > > >> >>>> in a very reasonable manner so far. Thanks
for this!
>>>> > > >> >>>>
>>>> > > >> >>>> – Ufuk
>>>> > > >> >>>>
>>>> > > >> >>>> On Wed, Oct 21, 2015 at 10:29 AM, Fabian
Hueske <
>>>> > fhueske@gmail.com
>>>> > > >
>>>> > > >> >>> wrote:
>>>> > > >> >>>>
>>>> > > >> >>>>> Thanks Max for checking the modifications
by the Google code
>>>> > > style.
>>>> > > >> >>>>> It is very good to know, that the
impact on the code base
>>>> would
>>>> > > not
>>>> > > >> be
>>>> > > >> >>> too
>>>> > > >> >>>>> massive. If the Google code style
would have touched almost
>>>> > every
>>>> > > >> >> line,
>>>> > > >> >>> I
>>>> > > >> >>>>> would have been in favor of converting
to spaces. However,
>>>> your
>>>> > > >> >>> assessment
>>>> > > >> >>>>> is a strong argument to continue
with tabs, IMO.
>>>> > > >> >>>>>
>>>> > > >> >>>>> Regarding the line length limit,
I personally find 100 chars
>>>> too
>>>> > > >> >> narrow
>>>> > > >> >>> but
>>>> > > >> >>>>> would be +1 for having a limit.
>>>> > > >> >>>>>
>>>> > > >> >>>>> +1 for discussing the Scala style
in a separate thread.
>>>> > > >> >>>>>
>>>> > > >> >>>>> Fabian
>>>> > > >> >>>>>
>>>> > > >> >>>>> 2015-10-20 18:12 GMT+02:00 Maximilian
Michels <mxm@apache.org
>>>> >:
>>>> > > >> >>>>>
>>>> > > >> >>>>>> I'm a little less excited about
this. You might not be aware
>>>> > but,
>>>> > > >> for
>>>> > > >> >>>>>> a large portion of the source
code, we already follow the
>>>> > Google
>>>> > > >> >> style
>>>> > > >> >>>>>> guide. The main changes will
be tabs->spaces and 80/100
>>>> > > characters
>>>> > > >> >>>>>> line limit.
>>>> > > >> >>>>>>
>>>> > > >> >>>>>> Out of curiosity, I ran the official
Google Style Checkstyle
>>>> > > >> >>>>>> configuration to confirm my suspicion:
>>>> > > >> >>>>>>
>>>> > > >> >>>>>>
>>>> > > >> >>>>>
>>>> > > >> >>>
>>>> > > >> >>
>>>> > > >>
>>>> > >
>>>> >
>>>> https://github.com/checkstyle/checkstyle/blob/master/src/main/resources/google_checks.xml
>>>> > > >> >>>>>> The changes are very little if
we turn off line length limit
>>>> > and
>>>> > > >> >>>>>> tabs-to-spaces conversion.
>>>> > > >> >>>>>>
>>>> > > >> >>>>>> There are some things I really
like about the Google style,
>>>> > e.g.
>>>> > > >> >> every
>>>> > > >> >>>>>> class has to have a JavaDoc and
spaces after keywords (can't
>>>> > > stand
>>>> > > >> if
>>>> > > >> >>>>>> there aren't any). I'm not sure
if we should change tabs to
>>>> > > spaces,
>>>> > > >> >>>>>> because it means touching almost
every single line of code.
>>>> > > However,
>>>> > > >> >>>>>> if we keep the tabs, we cannot
make use of the different
>>>> > > indention
>>>> > > >> >> for
>>>> > > >> >>>>>> case statements or wrapped lines...maybe
that's a compromise
>>>> we
>>>> > > can
>>>> > > >> >>>>>> live with.
>>>> > > >> >>>>>>
>>>> > > >> >>>>>> If we introduce the Google Style
for Java, will we also
>>>> impose
>>>> > a
>>>> > > >> >>>>>> stricter style check for Scala?
IMHO the line length is the
>>>> > > >> strictest
>>>> > > >> >>>>>> part of the Scala Checkstyle.
>>>> > > >> >>>>>>
>>>> > > >> >>>>>>
>>>> > > >> >>>>>> On Tue, Oct 20, 2015 at 4:14
PM, Henry Saputra <
>>>> > > >> >>> henry.saputra@gmail.com>
>>>> > > >> >>>>>> wrote:
>>>> > > >> >>>>>>> 1) yes. Been dancing this
issue for a while. Let's pull the
>>>> > > >> trigger.
>>>> > > >> >>>>> Did
>>>> > > >> >>>>>>> the exercise with Tachyon
while back and did help
>>>> readability
>>>> > > and
>>>> > > >> >>>>>>> homogeneity of code.
>>>> > > >> >>>>>>>
>>>> > > >> >>>>>>> 2) +1 for Google Java style
with documented exceptions and
>>>> > > >> >> explanation
>>>> > > >> >>>>> on
>>>> > > >> >>>>>>> why.
>>>> > > >> >>>>>>>
>>>> > > >> >>>>>>> On Tuesday, October 20, 2015,
Ufuk Celebi <uce@apache.org>
>>>> > > wrote:
>>>> > > >> >>>>>>>
>>>> > > >> >>>>>>>> DISCLAIMER: This is not
my personal idea, but a community
>>>> > > >> >> discussion
>>>> > > >> >>>>>> from
>>>> > > >> >>>>>>>> some time ago. Don't
kill the messenger.
>>>> > > >> >>>>>>>>
>>>> > > >> >>>>>>>> In March we were discussing
issues with heterogeneity of
>>>> the
>>>> > > code
>>>> > > >> >>> [1].
>>>> > > >> >>>>>> The
>>>> > > >> >>>>>>>> summary is that we had
a consensus to enforce a stricter
>>>> code
>>>> > > >> style
>>>> > > >> >>> on
>>>> > > >> >>>>>> our
>>>> > > >> >>>>>>>> Java code base in order
to make it easier to switch between
>>>> > > >> >> projects
>>>> > > >> >>>>>> and to
>>>> > > >> >>>>>>>> have clear rules for
new contributions. The main proposal
>>>> in
>>>> > > the
>>>> > > >> >> last
>>>> > > >> >>>>>>>> discussion was to go
with Google's Java code style. Not all
>>>> > > were
>>>> > > >> >>> fully
>>>> > > >> >>>>>>>> satisfied with this,
but still everyone agreed on some kind
>>>> > of
>>>> > > >> >> style.
>>>> > > >> >>>>>>>>
>>>> > > >> >>>>>>>> I think the upcoming
0.10 release is a good point to
>>>> finally
>>>> > go
>>>> > > >> >>>>> through
>>>> > > >> >>>>>>>> with these changes (right
after the release/branch-off).
>>>> > > >> >>>>>>>>
>>>> > > >> >>>>>>>> I propose to go with
Google's Java code style [2] as
>>>> proposed
>>>> > > >> >>> earlier.
>>>> > > >> >>>>>>>>
>>>> > > >> >>>>>>>> PROs:
>>>> > > >> >>>>>>>> - Clear style guide available
>>>> > > >> >>>>>>>> - Tooling like checkstyle
rules, IDE plugins already
>>>> > available
>>>> > > >> >>>>>>>>
>>>> > > >> >>>>>>>> CONs:
>>>> > > >> >>>>>>>> - Fully breaks our current
style
>>>> > > >> >>>>>>>>
>>>> > > >> >>>>>>>> The main problem with
this will be open pull requests,
>>>> which
>>>> > > will
>>>> > > >> >> be
>>>> > > >> >>>>>> harder
>>>> > > >> >>>>>>>> to merge after all the
changes. On the other hand, should
>>>> > pull
>>>> > > >> >>>>> requests
>>>> > > >> >>>>>>>> that have been open for
a long time block this? Most of the
>>>> > > >> >> important
>>>> > > >> >>>>>>>> changes will be merged
for the release anyways. I think in
>>>> > the
>>>> > > >> long
>>>> > > >> >>>>> run
>>>> > > >> >>>>>> we
>>>> > > >> >>>>>>>> will gain more than we
loose by this (more homogenous code,
>>>> > > clear
>>>> > > >> >>>>>> rules).
>>>> > > >> >>>>>>>> And it is questionable
whether we will ever be able to do
>>>> > such
>>>> > > a
>>>> > > >> >>>>> change
>>>> > > >> >>>>>> in
>>>> > > >> >>>>>>>> the future if we cannot
do it now. The project will most
>>>> > likely
>>>> > > >> >> grow
>>>> > > >> >>>>> and
>>>> > > >> >>>>>>>> attract more contributors,
at which point it will be even
>>>> > > harder
>>>> > > >> to
>>>> > > >> >>>>> do.
>>>> > > >> >>>>>>>>
>>>> > > >> >>>>>>>> Please make sure to answer
the following points in the
>>>> > > discussion:
>>>> > > >> >>>>>>>>
>>>> > > >> >>>>>>>> 1) Are you (still) in
favour of enforcing stricter rules on
>>>> > the
>>>> > > >> >> Java
>>>> > > >> >>>>>>>> codebase?
>>>> > > >> >>>>>>>>
>>>> > > >> >>>>>>>> 2) If yes, would you
be OK with the Google's Java code
>>>> style?
>>>> > > >> >>>>>>>>
>>>> > > >> >>>>>>>> – Ufuk
>>>> > > >> >>>>>>>>
>>>> > > >> >>>>>>>> [1]
>>>> > > >> >>>>>>>>
>>>> > > >> >>>>>>>>
>>>> > > >> >>>>>>
>>>> > > >> >>>>>
>>>> > > >> >>>
>>>> > > >> >>
>>>> > > >>
>>>> > >
>>>> >
>>>> http://mail-archives.apache.org/mod_mbox/flink-dev/201503.mbox/%3cCANC1h_voN0B5omNWZxcHTyzwHAKeGhbZVQUyK7S9o2A36b891Q@mail.gmail.com%3e
>>>> > > >> >>>>>>>>
>>>> > > >> >>>>>>>> [2] https://google.github.io/styleguide/javaguide.html
>>>> > > >> >>>>>>>>
>>>> > > >> >>>>>>
>>>> > > >> >>>>>
>>>> > > >> >>>>
>>>> > > >> >>>
>>>> > > >> >>>
>>>> > > >> >>
>>>> > > >> >
>>>> > > >>
>>>> > > >>
>>>> > >
>>>> >
>>>>

Mime
View raw message