flink-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Ufuk Celebi <...@apache.org>
Subject Re: [DISCUSS] Java code style
Date Fri, 23 Oct 2015 09:29:57 GMT
I think that we have two open questions now:

1. Line length

From the discussion so far, I think that no one wants 80 characters line length.

The final decision will be 100 vs. 120 characters. 120 characters is what we have right now
(for most parts), so it is fair to keep it that way, but enforce it (get rid of the longer
lines).

Is everyone OK with this?

2. Tabs vs. Spaces:

I hope I’m not misrepresenting someone with the following summary of positions.

Tabs:
- Robert
- Max
- Fabian
(- Stephan)

Spaces:
- Matthias
- Marton
- Till
- Gyula
- Henry
(- Stephan)

Let’s wrap the discussion up by focusing on this question.

What are the PROs/CONs for the respective approaches? If we went with the opposing approach,
would you voice a -1? E.g. would a “tabs person" -1 a "spaces decision" and vice versa?

– Ufuk

> On 23 Oct 2015, at 10:34, 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