[DISCUSS] Code style / checkstyle

classic Classic list List threaded Threaded
37 messages Options
12
Reply | Threaded
Open this post in threaded view
|

[DISCUSS] Code style / checkstyle

Dawid Wysakowicz
Hi,
I would like to resurrect the discussing ([1]
<http://apache-flink-mailing-list-archive.1008284.n3.nabble.com/Code-style-guideline-for-Scala-td7526.html>
, [2]
<http://apache-flink-mailing-list-archive.1008284.n3.nabble.com/Intellij-code-style-td11092.html>)
about creating unified code style(that could be imported to at least
IntelliJ and Eclipse) and corresponding stricter checkstyle rules.

I know that the hardest part is to adjust the existing code to the new
checkstyle rules. Do you believe it would be worth the effort? All
suggestions are welcome.
Reply | Threaded
Open this post in threaded view
|

Re: [DISCUSS] Code style / checkstyle

Kurt Young
+1 to provide a unified code style for both java and scala.

-1 to adjust all the existing code to the new style in one step. The code's
history contains very helpful information which can help
develops to understand why these codes are added, which jira is related.
This information is too valuable to lose. I think we can
do the reformat thing step by step, each time when the codes being changed,
we can adopt them to the new style.
IMHO this is also the reason why the unified code style is important.


Best,
Kurt

On Wed, Feb 22, 2017 at 5:50 AM, Dawid Wysakowicz <
[hidden email]> wrote:

> Hi,
> I would like to resurrect the discussing ([1]
> <http://apache-flink-mailing-list-archive.1008284.n3.
> nabble.com/Code-style-guideline-for-Scala-td7526.html>
> , [2]
> <http://apache-flink-mailing-list-archive.1008284.n3.
> nabble.com/Intellij-code-style-td11092.html>)
> about creating unified code style(that could be imported to at least
> IntelliJ and Eclipse) and corresponding stricter checkstyle rules.
>
> I know that the hardest part is to adjust the existing code to the new
> checkstyle rules. Do you believe it would be worth the effort? All
> suggestions are welcome.
>
Reply | Threaded
Open this post in threaded view
|

Re: [DISCUSS] Code style / checkstyle

Ufuk Celebi-2
Kurt's proposal sounds reasonable.

What about the following:
- We try to capture the current style in an code style configuration
(for IntelliJ and maybe Eclipse)
- We provide that on the website for contributors to download
- We don't enforce it, but new contributions and changes are free to
format with this style as changes happen

Practically speaking, this should not change much except maybe the
import order or whitespace after certain keywords, etc.


On Wed, Feb 22, 2017 at 4:48 AM, Kurt Young <[hidden email]> wrote:

> +1 to provide a unified code style for both java and scala.
>
> -1 to adjust all the existing code to the new style in one step. The code's
> history contains very helpful information which can help
> develops to understand why these codes are added, which jira is related.
> This information is too valuable to lose. I think we can
> do the reformat thing step by step, each time when the codes being changed,
> we can adopt them to the new style.
> IMHO this is also the reason why the unified code style is important.
>
>
> Best,
> Kurt
>
> On Wed, Feb 22, 2017 at 5:50 AM, Dawid Wysakowicz <
> [hidden email]> wrote:
>
>> Hi,
>> I would like to resurrect the discussing ([1]
>> <http://apache-flink-mailing-list-archive.1008284.n3.
>> nabble.com/Code-style-guideline-for-Scala-td7526.html>
>> , [2]
>> <http://apache-flink-mailing-list-archive.1008284.n3.
>> nabble.com/Intellij-code-style-td11092.html>)
>> about creating unified code style(that could be imported to at least
>> IntelliJ and Eclipse) and corresponding stricter checkstyle rules.
>>
>> I know that the hardest part is to adjust the existing code to the new
>> checkstyle rules. Do you believe it would be worth the effort? All
>> suggestions are welcome.
>>
Reply | Threaded
Open this post in threaded view
|

Re: [DISCUSS] Code style / checkstyle

Till Rohrmann
I think that not enforcing a code style is as good as not having any code
style to be honest. Having an IntelliJ or Eclipse profile is nice and some
people will probably use it, but my gut feeling is that the majority won't
notice it.

Cheers,
Till

On Wed, Feb 22, 2017 at 11:15 AM, Ufuk Celebi <[hidden email]> wrote:

> Kurt's proposal sounds reasonable.
>
> What about the following:
> - We try to capture the current style in an code style configuration
> (for IntelliJ and maybe Eclipse)
> - We provide that on the website for contributors to download
> - We don't enforce it, but new contributions and changes are free to
> format with this style as changes happen
>
> Practically speaking, this should not change much except maybe the
> import order or whitespace after certain keywords, etc.
>
>
> On Wed, Feb 22, 2017 at 4:48 AM, Kurt Young <[hidden email]> wrote:
> > +1 to provide a unified code style for both java and scala.
> >
> > -1 to adjust all the existing code to the new style in one step. The
> code's
> > history contains very helpful information which can help
> > develops to understand why these codes are added, which jira is related.
> > This information is too valuable to lose. I think we can
> > do the reformat thing step by step, each time when the codes being
> changed,
> > we can adopt them to the new style.
> > IMHO this is also the reason why the unified code style is important.
> >
> >
> > Best,
> > Kurt
> >
> > On Wed, Feb 22, 2017 at 5:50 AM, Dawid Wysakowicz <
> > [hidden email]> wrote:
> >
> >> Hi,
> >> I would like to resurrect the discussing ([1]
> >> <http://apache-flink-mailing-list-archive.1008284.n3.
> >> nabble.com/Code-style-guideline-for-Scala-td7526.html>
> >> , [2]
> >> <http://apache-flink-mailing-list-archive.1008284.n3.
> >> nabble.com/Intellij-code-style-td11092.html>)
> >> about creating unified code style(that could be imported to at least
> >> IntelliJ and Eclipse) and corresponding stricter checkstyle rules.
> >>
> >> I know that the hardest part is to adjust the existing code to the new
> >> checkstyle rules. Do you believe it would be worth the effort? All
> >> suggestions are welcome.
> >>
>
Reply | Threaded
Open this post in threaded view
|

Re: [DISCUSS] Code style / checkstyle

Chesnay Schepler-3
I agree with Till.

I would propose enforcing checkstyle on a subset of the modules,
basically those that are not
flink-runtime, flink-java, flink-streaming-java. These are the ones imo
where messing with the history
can be detrimental; for the others it isn't really important imo.
(Note that i excluded scala code since i don't know the state of
checkstyle compliance there)

For flink-runtime we could maybe (don't know if it is supported) enforce
checkstyle for all classes
under o.a.f.migration, so that at least the flip-6 code is covered.

Similarly, enforcing checkstyle for all tests should be fine as well.

Regards,
Chesnay

On 22.02.2017 11:48, Till Rohrmann wrote:

> I think that not enforcing a code style is as good as not having any code
> style to be honest. Having an IntelliJ or Eclipse profile is nice and some
> people will probably use it, but my gut feeling is that the majority won't
> notice it.
>
> Cheers,
> Till
>
> On Wed, Feb 22, 2017 at 11:15 AM, Ufuk Celebi <[hidden email]> wrote:
>
>> Kurt's proposal sounds reasonable.
>>
>> What about the following:
>> - We try to capture the current style in an code style configuration
>> (for IntelliJ and maybe Eclipse)
>> - We provide that on the website for contributors to download
>> - We don't enforce it, but new contributions and changes are free to
>> format with this style as changes happen
>>
>> Practically speaking, this should not change much except maybe the
>> import order or whitespace after certain keywords, etc.
>>
>>
>> On Wed, Feb 22, 2017 at 4:48 AM, Kurt Young <[hidden email]> wrote:
>>> +1 to provide a unified code style for both java and scala.
>>>
>>> -1 to adjust all the existing code to the new style in one step. The
>> code's
>>> history contains very helpful information which can help
>>> develops to understand why these codes are added, which jira is related.
>>> This information is too valuable to lose. I think we can
>>> do the reformat thing step by step, each time when the codes being
>> changed,
>>> we can adopt them to the new style.
>>> IMHO this is also the reason why the unified code style is important.
>>>
>>>
>>> Best,
>>> Kurt
>>>
>>> On Wed, Feb 22, 2017 at 5:50 AM, Dawid Wysakowicz <
>>> [hidden email]> wrote:
>>>
>>>> Hi,
>>>> I would like to resurrect the discussing ([1]
>>>> <http://apache-flink-mailing-list-archive.1008284.n3.
>>>> nabble.com/Code-style-guideline-for-Scala-td7526.html>
>>>> , [2]
>>>> <http://apache-flink-mailing-list-archive.1008284.n3.
>>>> nabble.com/Intellij-code-style-td11092.html>)
>>>> about creating unified code style(that could be imported to at least
>>>> IntelliJ and Eclipse) and corresponding stricter checkstyle rules.
>>>>
>>>> I know that the hardest part is to adjust the existing code to the new
>>>> checkstyle rules. Do you believe it would be worth the effort? All
>>>> suggestions are welcome.
>>>>

Reply | Threaded
Open this post in threaded view
|

Re: [DISCUSS] Code style / checkstyle

Dawid Wysakowicz
I also agree with Till and Chesnayl. Anyway as to "capture the current
style" I have some doubts if this is possible, as it changes file to file.

Chesnay's suggestion as to were enforce the checkstyle seems reasonable to
me, but I am quite new to the community :).
Enabling checkstyle for particular packages is possible.

2017-02-22 12:07 GMT+01:00 Chesnay Schepler <[hidden email]>:

> I agree with Till.
>
> I would propose enforcing checkstyle on a subset of the modules, basically
> those that are not
> flink-runtime, flink-java, flink-streaming-java. These are the ones imo
> where messing with the history
> can be detrimental; for the others it isn't really important imo.
> (Note that i excluded scala code since i don't know the state of
> checkstyle compliance there)
>
> For flink-runtime we could maybe (don't know if it is supported) enforce
> checkstyle for all classes
> under o.a.f.migration, so that at least the flip-6 code is covered.
>
> Similarly, enforcing checkstyle for all tests should be fine as well.
>
> Regards,
> Chesnay
>
>
> On 22.02.2017 11:48, Till Rohrmann wrote:
>
>> I think that not enforcing a code style is as good as not having any code
>> style to be honest. Having an IntelliJ or Eclipse profile is nice and some
>> people will probably use it, but my gut feeling is that the majority won't
>> notice it.
>>
>> Cheers,
>> Till
>>
>> On Wed, Feb 22, 2017 at 11:15 AM, Ufuk Celebi <[hidden email]> wrote:
>>
>> Kurt's proposal sounds reasonable.
>>>
>>> What about the following:
>>> - We try to capture the current style in an code style configuration
>>> (for IntelliJ and maybe Eclipse)
>>> - We provide that on the website for contributors to download
>>> - We don't enforce it, but new contributions and changes are free to
>>> format with this style as changes happen
>>>
>>> Practically speaking, this should not change much except maybe the
>>> import order or whitespace after certain keywords, etc.
>>>
>>>
>>> On Wed, Feb 22, 2017 at 4:48 AM, Kurt Young <[hidden email]> wrote:
>>>
>>>> +1 to provide a unified code style for both java and scala.
>>>>
>>>> -1 to adjust all the existing code to the new style in one step. The
>>>>
>>> code's
>>>
>>>> history contains very helpful information which can help
>>>> develops to understand why these codes are added, which jira is related.
>>>> This information is too valuable to lose. I think we can
>>>> do the reformat thing step by step, each time when the codes being
>>>>
>>> changed,
>>>
>>>> we can adopt them to the new style.
>>>> IMHO this is also the reason why the unified code style is important.
>>>>
>>>>
>>>> Best,
>>>> Kurt
>>>>
>>>> On Wed, Feb 22, 2017 at 5:50 AM, Dawid Wysakowicz <
>>>> [hidden email]> wrote:
>>>>
>>>> Hi,
>>>>> I would like to resurrect the discussing ([1]
>>>>> <http://apache-flink-mailing-list-archive.1008284.n3.
>>>>> nabble.com/Code-style-guideline-for-Scala-td7526.html>
>>>>> , [2]
>>>>> <http://apache-flink-mailing-list-archive.1008284.n3.
>>>>> nabble.com/Intellij-code-style-td11092.html>)
>>>>> about creating unified code style(that could be imported to at least
>>>>> IntelliJ and Eclipse) and corresponding stricter checkstyle rules.
>>>>>
>>>>> I know that the hardest part is to adjust the existing code to the new
>>>>> checkstyle rules. Do you believe it would be worth the effort? All
>>>>> suggestions are welcome.
>>>>>
>>>>>
>
Reply | Threaded
Open this post in threaded view
|

Re: [DISCUSS] Code style / checkstyle

Greg Hogan
Will not the code style be applied on save to any user-modified file? So
this will clutter PRs and overwrite history.

On Wed, Feb 22, 2017 at 6:19 AM, Dawid Wysakowicz <
[hidden email]> wrote:

> I also agree with Till and Chesnayl. Anyway as to "capture the current
> style" I have some doubts if this is possible, as it changes file to file.
>
> Chesnay's suggestion as to were enforce the checkstyle seems reasonable to
> me, but I am quite new to the community :).
> Enabling checkstyle for particular packages is possible.
>
> 2017-02-22 12:07 GMT+01:00 Chesnay Schepler <[hidden email]>:
>
> > I agree with Till.
> >
> > I would propose enforcing checkstyle on a subset of the modules,
> basically
> > those that are not
> > flink-runtime, flink-java, flink-streaming-java. These are the ones imo
> > where messing with the history
> > can be detrimental; for the others it isn't really important imo.
> > (Note that i excluded scala code since i don't know the state of
> > checkstyle compliance there)
> >
> > For flink-runtime we could maybe (don't know if it is supported) enforce
> > checkstyle for all classes
> > under o.a.f.migration, so that at least the flip-6 code is covered.
> >
> > Similarly, enforcing checkstyle for all tests should be fine as well.
> >
> > Regards,
> > Chesnay
> >
> >
> > On 22.02.2017 11:48, Till Rohrmann wrote:
> >
> >> I think that not enforcing a code style is as good as not having any
> code
> >> style to be honest. Having an IntelliJ or Eclipse profile is nice and
> some
> >> people will probably use it, but my gut feeling is that the majority
> won't
> >> notice it.
> >>
> >> Cheers,
> >> Till
> >>
> >> On Wed, Feb 22, 2017 at 11:15 AM, Ufuk Celebi <[hidden email]> wrote:
> >>
> >> Kurt's proposal sounds reasonable.
> >>>
> >>> What about the following:
> >>> - We try to capture the current style in an code style configuration
> >>> (for IntelliJ and maybe Eclipse)
> >>> - We provide that on the website for contributors to download
> >>> - We don't enforce it, but new contributions and changes are free to
> >>> format with this style as changes happen
> >>>
> >>> Practically speaking, this should not change much except maybe the
> >>> import order or whitespace after certain keywords, etc.
> >>>
> >>>
> >>> On Wed, Feb 22, 2017 at 4:48 AM, Kurt Young <[hidden email]> wrote:
> >>>
> >>>> +1 to provide a unified code style for both java and scala.
> >>>>
> >>>> -1 to adjust all the existing code to the new style in one step. The
> >>>>
> >>> code's
> >>>
> >>>> history contains very helpful information which can help
> >>>> develops to understand why these codes are added, which jira is
> related.
> >>>> This information is too valuable to lose. I think we can
> >>>> do the reformat thing step by step, each time when the codes being
> >>>>
> >>> changed,
> >>>
> >>>> we can adopt them to the new style.
> >>>> IMHO this is also the reason why the unified code style is important.
> >>>>
> >>>>
> >>>> Best,
> >>>> Kurt
> >>>>
> >>>> On Wed, Feb 22, 2017 at 5:50 AM, Dawid Wysakowicz <
> >>>> [hidden email]> wrote:
> >>>>
> >>>> Hi,
> >>>>> I would like to resurrect the discussing ([1]
> >>>>> <http://apache-flink-mailing-list-archive.1008284.n3.
> >>>>> nabble.com/Code-style-guideline-for-Scala-td7526.html>
> >>>>> , [2]
> >>>>> <http://apache-flink-mailing-list-archive.1008284.n3.
> >>>>> nabble.com/Intellij-code-style-td11092.html>)
> >>>>> about creating unified code style(that could be imported to at least
> >>>>> IntelliJ and Eclipse) and corresponding stricter checkstyle rules.
> >>>>>
> >>>>> I know that the hardest part is to adjust the existing code to the
> new
> >>>>> checkstyle rules. Do you believe it would be worth the effort? All
> >>>>> suggestions are welcome.
> >>>>>
> >>>>>
> >
>
Reply | Threaded
Open this post in threaded view
|

Re: [DISCUSS] Code style / checkstyle

Chesnay Schepler-3
For file where we don't enforce checkstyle things should work they way
they do right now.

Turn off auto-formatting, and only format code that you touched and
that's it. For these
modification we will have to check them manually in the PRs as we do now.

On 22.02.2017 16:22, Greg Hogan wrote:

> Will not the code style be applied on save to any user-modified file? So
> this will clutter PRs and overwrite history.
>
> On Wed, Feb 22, 2017 at 6:19 AM, Dawid Wysakowicz <
> [hidden email]> wrote:
>
>> I also agree with Till and Chesnayl. Anyway as to "capture the current
>> style" I have some doubts if this is possible, as it changes file to file.
>>
>> Chesnay's suggestion as to were enforce the checkstyle seems reasonable to
>> me, but I am quite new to the community :).
>> Enabling checkstyle for particular packages is possible.
>>
>> 2017-02-22 12:07 GMT+01:00 Chesnay Schepler <[hidden email]>:
>>
>>> I agree with Till.
>>>
>>> I would propose enforcing checkstyle on a subset of the modules,
>> basically
>>> those that are not
>>> flink-runtime, flink-java, flink-streaming-java. These are the ones imo
>>> where messing with the history
>>> can be detrimental; for the others it isn't really important imo.
>>> (Note that i excluded scala code since i don't know the state of
>>> checkstyle compliance there)
>>>
>>> For flink-runtime we could maybe (don't know if it is supported) enforce
>>> checkstyle for all classes
>>> under o.a.f.migration, so that at least the flip-6 code is covered.
>>>
>>> Similarly, enforcing checkstyle for all tests should be fine as well.
>>>
>>> Regards,
>>> Chesnay
>>>
>>>
>>> On 22.02.2017 11:48, Till Rohrmann wrote:
>>>
>>>> I think that not enforcing a code style is as good as not having any
>> code
>>>> style to be honest. Having an IntelliJ or Eclipse profile is nice and
>> some
>>>> people will probably use it, but my gut feeling is that the majority
>> won't
>>>> notice it.
>>>>
>>>> Cheers,
>>>> Till
>>>>
>>>> On Wed, Feb 22, 2017 at 11:15 AM, Ufuk Celebi <[hidden email]> wrote:
>>>>
>>>> Kurt's proposal sounds reasonable.
>>>>> What about the following:
>>>>> - We try to capture the current style in an code style configuration
>>>>> (for IntelliJ and maybe Eclipse)
>>>>> - We provide that on the website for contributors to download
>>>>> - We don't enforce it, but new contributions and changes are free to
>>>>> format with this style as changes happen
>>>>>
>>>>> Practically speaking, this should not change much except maybe the
>>>>> import order or whitespace after certain keywords, etc.
>>>>>
>>>>>
>>>>> On Wed, Feb 22, 2017 at 4:48 AM, Kurt Young <[hidden email]> wrote:
>>>>>
>>>>>> +1 to provide a unified code style for both java and scala.
>>>>>>
>>>>>> -1 to adjust all the existing code to the new style in one step. The
>>>>>>
>>>>> code's
>>>>>
>>>>>> history contains very helpful information which can help
>>>>>> develops to understand why these codes are added, which jira is
>> related.
>>>>>> This information is too valuable to lose. I think we can
>>>>>> do the reformat thing step by step, each time when the codes being
>>>>>>
>>>>> changed,
>>>>>
>>>>>> we can adopt them to the new style.
>>>>>> IMHO this is also the reason why the unified code style is important.
>>>>>>
>>>>>>
>>>>>> Best,
>>>>>> Kurt
>>>>>>
>>>>>> On Wed, Feb 22, 2017 at 5:50 AM, Dawid Wysakowicz <
>>>>>> [hidden email]> wrote:
>>>>>>
>>>>>> Hi,
>>>>>>> I would like to resurrect the discussing ([1]
>>>>>>> <http://apache-flink-mailing-list-archive.1008284.n3.
>>>>>>> nabble.com/Code-style-guideline-for-Scala-td7526.html>
>>>>>>> , [2]
>>>>>>> <http://apache-flink-mailing-list-archive.1008284.n3.
>>>>>>> nabble.com/Intellij-code-style-td11092.html>)
>>>>>>> about creating unified code style(that could be imported to at least
>>>>>>> IntelliJ and Eclipse) and corresponding stricter checkstyle rules.
>>>>>>>
>>>>>>> I know that the hardest part is to adjust the existing code to the
>> new
>>>>>>> checkstyle rules. Do you believe it would be worth the effort? All
>>>>>>> suggestions are welcome.
>>>>>>>
>>>>>>>

Reply | Threaded
Open this post in threaded view
|

Re: [DISCUSS] Code style / checkstyle

Dawid Wysakowicz
So how about preparing a code style and corresponding checkstyle and
enabling it gradually module by module whenever shepherd/commiter with
expertise in a module will have time to introduce/check such a change?
Maybe it will make the "snowball effect" happen?
I agree there is no point in preparing code style/checkstyle until it will
be introduced somewhere. I will be willing to work on the checkstyle if
some volunteering modules appear.

2017-02-22 17:09 GMT+01:00 Chesnay Schepler <[hidden email]>:

> For file where we don't enforce checkstyle things should work they way
> they do right now.
>
> Turn off auto-formatting, and only format code that you touched and that's
> it. For these
> modification we will have to check them manually in the PRs as we do now.
>
>
> On 22.02.2017 16:22, Greg Hogan wrote:
>
>> Will not the code style be applied on save to any user-modified file? So
>> this will clutter PRs and overwrite history.
>>
>> On Wed, Feb 22, 2017 at 6:19 AM, Dawid Wysakowicz <
>> [hidden email]> wrote:
>>
>> I also agree with Till and Chesnayl. Anyway as to "capture the current
>>> style" I have some doubts if this is possible, as it changes file to
>>> file.
>>>
>>> Chesnay's suggestion as to were enforce the checkstyle seems reasonable
>>> to
>>> me, but I am quite new to the community :).
>>> Enabling checkstyle for particular packages is possible.
>>>
>>> 2017-02-22 12:07 GMT+01:00 Chesnay Schepler <[hidden email]>:
>>>
>>> I agree with Till.
>>>>
>>>> I would propose enforcing checkstyle on a subset of the modules,
>>>>
>>> basically
>>>
>>>> those that are not
>>>> flink-runtime, flink-java, flink-streaming-java. These are the ones imo
>>>> where messing with the history
>>>> can be detrimental; for the others it isn't really important imo.
>>>> (Note that i excluded scala code since i don't know the state of
>>>> checkstyle compliance there)
>>>>
>>>> For flink-runtime we could maybe (don't know if it is supported) enforce
>>>> checkstyle for all classes
>>>> under o.a.f.migration, so that at least the flip-6 code is covered.
>>>>
>>>> Similarly, enforcing checkstyle for all tests should be fine as well.
>>>>
>>>> Regards,
>>>> Chesnay
>>>>
>>>>
>>>> On 22.02.2017 11:48, Till Rohrmann wrote:
>>>>
>>>> I think that not enforcing a code style is as good as not having any
>>>>>
>>>> code
>>>
>>>> style to be honest. Having an IntelliJ or Eclipse profile is nice and
>>>>>
>>>> some
>>>
>>>> people will probably use it, but my gut feeling is that the majority
>>>>>
>>>> won't
>>>
>>>> notice it.
>>>>>
>>>>> Cheers,
>>>>> Till
>>>>>
>>>>> On Wed, Feb 22, 2017 at 11:15 AM, Ufuk Celebi <[hidden email]> wrote:
>>>>>
>>>>> Kurt's proposal sounds reasonable.
>>>>>
>>>>>> What about the following:
>>>>>> - We try to capture the current style in an code style configuration
>>>>>> (for IntelliJ and maybe Eclipse)
>>>>>> - We provide that on the website for contributors to download
>>>>>> - We don't enforce it, but new contributions and changes are free to
>>>>>> format with this style as changes happen
>>>>>>
>>>>>> Practically speaking, this should not change much except maybe the
>>>>>> import order or whitespace after certain keywords, etc.
>>>>>>
>>>>>>
>>>>>> On Wed, Feb 22, 2017 at 4:48 AM, Kurt Young <[hidden email]> wrote:
>>>>>>
>>>>>> +1 to provide a unified code style for both java and scala.
>>>>>>>
>>>>>>> -1 to adjust all the existing code to the new style in one step. The
>>>>>>>
>>>>>>> code's
>>>>>>
>>>>>> history contains very helpful information which can help
>>>>>>> develops to understand why these codes are added, which jira is
>>>>>>>
>>>>>> related.
>>>
>>>> This information is too valuable to lose. I think we can
>>>>>>> do the reformat thing step by step, each time when the codes being
>>>>>>>
>>>>>>> changed,
>>>>>>
>>>>>> we can adopt them to the new style.
>>>>>>> IMHO this is also the reason why the unified code style is important.
>>>>>>>
>>>>>>>
>>>>>>> Best,
>>>>>>> Kurt
>>>>>>>
>>>>>>> On Wed, Feb 22, 2017 at 5:50 AM, Dawid Wysakowicz <
>>>>>>> [hidden email]> wrote:
>>>>>>>
>>>>>>> Hi,
>>>>>>>
>>>>>>>> I would like to resurrect the discussing ([1]
>>>>>>>> <http://apache-flink-mailing-list-archive.1008284.n3.
>>>>>>>> nabble.com/Code-style-guideline-for-Scala-td7526.html>
>>>>>>>> , [2]
>>>>>>>> <http://apache-flink-mailing-list-archive.1008284.n3.
>>>>>>>> nabble.com/Intellij-code-style-td11092.html>)
>>>>>>>> about creating unified code style(that could be imported to at least
>>>>>>>> IntelliJ and Eclipse) and corresponding stricter checkstyle rules.
>>>>>>>>
>>>>>>>> I know that the hardest part is to adjust the existing code to the
>>>>>>>>
>>>>>>> new
>>>
>>>> checkstyle rules. Do you believe it would be worth the effort? All
>>>>>>>> suggestions are welcome.
>>>>>>>>
>>>>>>>>
>>>>>>>>
>
Reply | Threaded
Open this post in threaded view
|

Re: [DISCUSS] Code style / checkstyle

Aljoscha Krettek-2
If we go for a codestyle/checkstyle I would suggest to use the Google
style. This already has checkstyle, IntelliJ style, Eclipse style and a
code formatting tool and is well established. However, some people will not
like this style. In general, I think we will never manage to find a style
that all people will like.

On Wed, 22 Feb 2017 at 18:36 Dawid Wysakowicz <[hidden email]>
wrote:

> So how about preparing a code style and corresponding checkstyle and
> enabling it gradually module by module whenever shepherd/commiter with
> expertise in a module will have time to introduce/check such a change?
> Maybe it will make the "snowball effect" happen?
> I agree there is no point in preparing code style/checkstyle until it will
> be introduced somewhere. I will be willing to work on the checkstyle if
> some volunteering modules appear.
>
> 2017-02-22 17:09 GMT+01:00 Chesnay Schepler <[hidden email]>:
>
> > For file where we don't enforce checkstyle things should work they way
> > they do right now.
> >
> > Turn off auto-formatting, and only format code that you touched and
> that's
> > it. For these
> > modification we will have to check them manually in the PRs as we do now.
> >
> >
> > On 22.02.2017 16:22, Greg Hogan wrote:
> >
> >> Will not the code style be applied on save to any user-modified file? So
> >> this will clutter PRs and overwrite history.
> >>
> >> On Wed, Feb 22, 2017 at 6:19 AM, Dawid Wysakowicz <
> >> [hidden email]> wrote:
> >>
> >> I also agree with Till and Chesnayl. Anyway as to "capture the current
> >>> style" I have some doubts if this is possible, as it changes file to
> >>> file.
> >>>
> >>> Chesnay's suggestion as to were enforce the checkstyle seems reasonable
> >>> to
> >>> me, but I am quite new to the community :).
> >>> Enabling checkstyle for particular packages is possible.
> >>>
> >>> 2017-02-22 12:07 GMT+01:00 Chesnay Schepler <[hidden email]>:
> >>>
> >>> I agree with Till.
> >>>>
> >>>> I would propose enforcing checkstyle on a subset of the modules,
> >>>>
> >>> basically
> >>>
> >>>> those that are not
> >>>> flink-runtime, flink-java, flink-streaming-java. These are the ones
> imo
> >>>> where messing with the history
> >>>> can be detrimental; for the others it isn't really important imo.
> >>>> (Note that i excluded scala code since i don't know the state of
> >>>> checkstyle compliance there)
> >>>>
> >>>> For flink-runtime we could maybe (don't know if it is supported)
> enforce
> >>>> checkstyle for all classes
> >>>> under o.a.f.migration, so that at least the flip-6 code is covered.
> >>>>
> >>>> Similarly, enforcing checkstyle for all tests should be fine as well.
> >>>>
> >>>> Regards,
> >>>> Chesnay
> >>>>
> >>>>
> >>>> On 22.02.2017 11:48, Till Rohrmann wrote:
> >>>>
> >>>> I think that not enforcing a code style is as good as not having any
> >>>>>
> >>>> code
> >>>
> >>>> style to be honest. Having an IntelliJ or Eclipse profile is nice and
> >>>>>
> >>>> some
> >>>
> >>>> people will probably use it, but my gut feeling is that the majority
> >>>>>
> >>>> won't
> >>>
> >>>> notice it.
> >>>>>
> >>>>> Cheers,
> >>>>> Till
> >>>>>
> >>>>> On Wed, Feb 22, 2017 at 11:15 AM, Ufuk Celebi <[hidden email]>
> wrote:
> >>>>>
> >>>>> Kurt's proposal sounds reasonable.
> >>>>>
> >>>>>> What about the following:
> >>>>>> - We try to capture the current style in an code style configuration
> >>>>>> (for IntelliJ and maybe Eclipse)
> >>>>>> - We provide that on the website for contributors to download
> >>>>>> - We don't enforce it, but new contributions and changes are free to
> >>>>>> format with this style as changes happen
> >>>>>>
> >>>>>> Practically speaking, this should not change much except maybe the
> >>>>>> import order or whitespace after certain keywords, etc.
> >>>>>>
> >>>>>>
> >>>>>> On Wed, Feb 22, 2017 at 4:48 AM, Kurt Young <[hidden email]>
> wrote:
> >>>>>>
> >>>>>> +1 to provide a unified code style for both java and scala.
> >>>>>>>
> >>>>>>> -1 to adjust all the existing code to the new style in one step.
> The
> >>>>>>>
> >>>>>>> code's
> >>>>>>
> >>>>>> history contains very helpful information which can help
> >>>>>>> develops to understand why these codes are added, which jira is
> >>>>>>>
> >>>>>> related.
> >>>
> >>>> This information is too valuable to lose. I think we can
> >>>>>>> do the reformat thing step by step, each time when the codes being
> >>>>>>>
> >>>>>>> changed,
> >>>>>>
> >>>>>> we can adopt them to the new style.
> >>>>>>> IMHO this is also the reason why the unified code style is
> important.
> >>>>>>>
> >>>>>>>
> >>>>>>> Best,
> >>>>>>> Kurt
> >>>>>>>
> >>>>>>> On Wed, Feb 22, 2017 at 5:50 AM, Dawid Wysakowicz <
> >>>>>>> [hidden email]> wrote:
> >>>>>>>
> >>>>>>> Hi,
> >>>>>>>
> >>>>>>>> I would like to resurrect the discussing ([1]
> >>>>>>>> <http://apache-flink-mailing-list-archive.1008284.n3.
> >>>>>>>> nabble.com/Code-style-guideline-for-Scala-td7526.html>
> >>>>>>>> , [2]
> >>>>>>>> <http://apache-flink-mailing-list-archive.1008284.n3.
> >>>>>>>> nabble.com/Intellij-code-style-td11092.html>)
> >>>>>>>> about creating unified code style(that could be imported to at
> least
> >>>>>>>> IntelliJ and Eclipse) and corresponding stricter checkstyle rules.
> >>>>>>>>
> >>>>>>>> I know that the hardest part is to adjust the existing code to the
> >>>>>>>>
> >>>>>>> new
> >>>
> >>>> checkstyle rules. Do you believe it would be worth the effort? All
> >>>>>>>> suggestions are welcome.
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>
> >
>
Reply | Threaded
Open this post in threaded view
|

Re: [DISCUSS] Code style / checkstyle

Jinkui Shi
Thanks to discuss this problem again.
1. Google checkstyle is good for java.
2. scala check style is here [1]
3. We can make a Flink plan contain issues, one sub-issue one rule. Resolve this in short time.

Current code style may be historical accumulate. If we don’t normalize the code step by step, new code will also follow bad style without strict checking.
We can adjust the exist code once one rule. But there will be lots of conflict in the not merged pull requests. Need all the pull request resolve the conflict manually.
Resolve long term trouble in short time:)


[1]  http://www.scalastyle.org/ <http://www.scalastyle.org/>

> On Feb 23, 2017, at 23:31, Aljoscha Krettek <[hidden email]> wrote:
>
> If we go for a codestyle/checkstyle I would suggest to use the Google
> style. This already has checkstyle, IntelliJ style, Eclipse style and a
> code formatting tool and is well established. However, some people will not
> like this style. In general, I think we will never manage to find a style
> that all people will like.
>
> On Wed, 22 Feb 2017 at 18:36 Dawid Wysakowicz <[hidden email]>
> wrote:
>
>> So how about preparing a code style and corresponding checkstyle and
>> enabling it gradually module by module whenever shepherd/commiter with
>> expertise in a module will have time to introduce/check such a change?
>> Maybe it will make the "snowball effect" happen?
>> I agree there is no point in preparing code style/checkstyle until it will
>> be introduced somewhere. I will be willing to work on the checkstyle if
>> some volunteering modules appear.
>>
>> 2017-02-22 17:09 GMT+01:00 Chesnay Schepler <[hidden email]>:
>>
>>> For file where we don't enforce checkstyle things should work they way
>>> they do right now.
>>>
>>> Turn off auto-formatting, and only format code that you touched and
>> that's
>>> it. For these
>>> modification we will have to check them manually in the PRs as we do now.
>>>
>>>
>>> On 22.02.2017 16:22, Greg Hogan wrote:
>>>
>>>> Will not the code style be applied on save to any user-modified file? So
>>>> this will clutter PRs and overwrite history.
>>>>
>>>> On Wed, Feb 22, 2017 at 6:19 AM, Dawid Wysakowicz <
>>>> [hidden email]> wrote:
>>>>
>>>> I also agree with Till and Chesnayl. Anyway as to "capture the current
>>>>> style" I have some doubts if this is possible, as it changes file to
>>>>> file.
>>>>>
>>>>> Chesnay's suggestion as to were enforce the checkstyle seems reasonable
>>>>> to
>>>>> me, but I am quite new to the community :).
>>>>> Enabling checkstyle for particular packages is possible.
>>>>>
>>>>> 2017-02-22 12:07 GMT+01:00 Chesnay Schepler <[hidden email]>:
>>>>>
>>>>> I agree with Till.
>>>>>>
>>>>>> I would propose enforcing checkstyle on a subset of the modules,
>>>>>>
>>>>> basically
>>>>>
>>>>>> those that are not
>>>>>> flink-runtime, flink-java, flink-streaming-java. These are the ones
>> imo
>>>>>> where messing with the history
>>>>>> can be detrimental; for the others it isn't really important imo.
>>>>>> (Note that i excluded scala code since i don't know the state of
>>>>>> checkstyle compliance there)
>>>>>>
>>>>>> For flink-runtime we could maybe (don't know if it is supported)
>> enforce
>>>>>> checkstyle for all classes
>>>>>> under o.a.f.migration, so that at least the flip-6 code is covered.
>>>>>>
>>>>>> Similarly, enforcing checkstyle for all tests should be fine as well.
>>>>>>
>>>>>> Regards,
>>>>>> Chesnay
>>>>>>
>>>>>>
>>>>>> On 22.02.2017 11:48, Till Rohrmann wrote:
>>>>>>
>>>>>> I think that not enforcing a code style is as good as not having any
>>>>>>>
>>>>>> code
>>>>>
>>>>>> style to be honest. Having an IntelliJ or Eclipse profile is nice and
>>>>>>>
>>>>>> some
>>>>>
>>>>>> people will probably use it, but my gut feeling is that the majority
>>>>>>>
>>>>>> won't
>>>>>
>>>>>> notice it.
>>>>>>>
>>>>>>> Cheers,
>>>>>>> Till
>>>>>>>
>>>>>>> On Wed, Feb 22, 2017 at 11:15 AM, Ufuk Celebi <[hidden email]>
>> wrote:
>>>>>>>
>>>>>>> Kurt's proposal sounds reasonable.
>>>>>>>
>>>>>>>> What about the following:
>>>>>>>> - We try to capture the current style in an code style configuration
>>>>>>>> (for IntelliJ and maybe Eclipse)
>>>>>>>> - We provide that on the website for contributors to download
>>>>>>>> - We don't enforce it, but new contributions and changes are free to
>>>>>>>> format with this style as changes happen
>>>>>>>>
>>>>>>>> Practically speaking, this should not change much except maybe the
>>>>>>>> import order or whitespace after certain keywords, etc.
>>>>>>>>
>>>>>>>>
>>>>>>>> On Wed, Feb 22, 2017 at 4:48 AM, Kurt Young <[hidden email]>
>> wrote:
>>>>>>>>
>>>>>>>> +1 to provide a unified code style for both java and scala.
>>>>>>>>>
>>>>>>>>> -1 to adjust all the existing code to the new style in one step.
>> The
>>>>>>>>>
>>>>>>>>> code's
>>>>>>>>
>>>>>>>> history contains very helpful information which can help
>>>>>>>>> develops to understand why these codes are added, which jira is
>>>>>>>>>
>>>>>>>> related.
>>>>>
>>>>>> This information is too valuable to lose. I think we can
>>>>>>>>> do the reformat thing step by step, each time when the codes being
>>>>>>>>>
>>>>>>>>> changed,
>>>>>>>>
>>>>>>>> we can adopt them to the new style.
>>>>>>>>> IMHO this is also the reason why the unified code style is
>> important.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Best,
>>>>>>>>> Kurt
>>>>>>>>>
>>>>>>>>> On Wed, Feb 22, 2017 at 5:50 AM, Dawid Wysakowicz <
>>>>>>>>> [hidden email]> wrote:
>>>>>>>>>
>>>>>>>>> Hi,
>>>>>>>>>
>>>>>>>>>> I would like to resurrect the discussing ([1]
>>>>>>>>>> <http://apache-flink-mailing-list-archive.1008284.n3.
>>>>>>>>>> nabble.com/Code-style-guideline-for-Scala-td7526.html>
>>>>>>>>>> , [2]
>>>>>>>>>> <http://apache-flink-mailing-list-archive.1008284.n3.
>>>>>>>>>> nabble.com/Intellij-code-style-td11092.html>)
>>>>>>>>>> about creating unified code style(that could be imported to at
>> least
>>>>>>>>>> IntelliJ and Eclipse) and corresponding stricter checkstyle rules.
>>>>>>>>>>
>>>>>>>>>> I know that the hardest part is to adjust the existing code to the
>>>>>>>>>>
>>>>>>>>> new
>>>>>
>>>>>> checkstyle rules. Do you believe it would be worth the effort? All
>>>>>>>>>> suggestions are welcome.
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>
>>

Reply | Threaded
Open this post in threaded view
|

Re: [DISCUSS] Code style / checkstyle

Fabian Hueske-2
We have discussed this issue several times in the past and never got around
to do it.

Although I agree that it would be nice to have a stricter code style, I
don't think we should introduce a code style.
IMO, at this point the costs (losing the history, PR hassle, etc.) would be
too high compared to the benefits (that I am aware of).

I agree with Till that encouraging a code style without enforcing it does
not make a lot of sense.
If we enforce it, we need to touch all files and PRs.

On the other hand, it's not clear to me how much we would gain with a code
style and what's the cost of not having one.

Best, Fabian





2017-02-23 17:52 GMT+01:00 Jinkui Shi <[hidden email]>:

> Thanks to discuss this problem again.
> 1. Google checkstyle is good for java.
> 2. scala check style is here [1]
> 3. We can make a Flink plan contain issues, one sub-issue one rule.
> Resolve this in short time.
>
> Current code style may be historical accumulate. If we don’t normalize the
> code step by step, new code will also follow bad style without strict
> checking.
> We can adjust the exist code once one rule. But there will be lots of
> conflict in the not merged pull requests. Need all the pull request resolve
> the conflict manually.
> Resolve long term trouble in short time:)
>
>
> [1]  http://www.scalastyle.org/ <http://www.scalastyle.org/>
> > On Feb 23, 2017, at 23:31, Aljoscha Krettek <[hidden email]> wrote:
> >
> > If we go for a codestyle/checkstyle I would suggest to use the Google
> > style. This already has checkstyle, IntelliJ style, Eclipse style and a
> > code formatting tool and is well established. However, some people will
> not
> > like this style. In general, I think we will never manage to find a style
> > that all people will like.
> >
> > On Wed, 22 Feb 2017 at 18:36 Dawid Wysakowicz <
> [hidden email]>
> > wrote:
> >
> >> So how about preparing a code style and corresponding checkstyle and
> >> enabling it gradually module by module whenever shepherd/commiter with
> >> expertise in a module will have time to introduce/check such a change?
> >> Maybe it will make the "snowball effect" happen?
> >> I agree there is no point in preparing code style/checkstyle until it
> will
> >> be introduced somewhere. I will be willing to work on the checkstyle if
> >> some volunteering modules appear.
> >>
> >> 2017-02-22 17:09 GMT+01:00 Chesnay Schepler <[hidden email]>:
> >>
> >>> For file where we don't enforce checkstyle things should work they way
> >>> they do right now.
> >>>
> >>> Turn off auto-formatting, and only format code that you touched and
> >> that's
> >>> it. For these
> >>> modification we will have to check them manually in the PRs as we do
> now.
> >>>
> >>>
> >>> On 22.02.2017 16:22, Greg Hogan wrote:
> >>>
> >>>> Will not the code style be applied on save to any user-modified file?
> So
> >>>> this will clutter PRs and overwrite history.
> >>>>
> >>>> On Wed, Feb 22, 2017 at 6:19 AM, Dawid Wysakowicz <
> >>>> [hidden email]> wrote:
> >>>>
> >>>> I also agree with Till and Chesnayl. Anyway as to "capture the current
> >>>>> style" I have some doubts if this is possible, as it changes file to
> >>>>> file.
> >>>>>
> >>>>> Chesnay's suggestion as to were enforce the checkstyle seems
> reasonable
> >>>>> to
> >>>>> me, but I am quite new to the community :).
> >>>>> Enabling checkstyle for particular packages is possible.
> >>>>>
> >>>>> 2017-02-22 12:07 GMT+01:00 Chesnay Schepler <[hidden email]>:
> >>>>>
> >>>>> I agree with Till.
> >>>>>>
> >>>>>> I would propose enforcing checkstyle on a subset of the modules,
> >>>>>>
> >>>>> basically
> >>>>>
> >>>>>> those that are not
> >>>>>> flink-runtime, flink-java, flink-streaming-java. These are the ones
> >> imo
> >>>>>> where messing with the history
> >>>>>> can be detrimental; for the others it isn't really important imo.
> >>>>>> (Note that i excluded scala code since i don't know the state of
> >>>>>> checkstyle compliance there)
> >>>>>>
> >>>>>> For flink-runtime we could maybe (don't know if it is supported)
> >> enforce
> >>>>>> checkstyle for all classes
> >>>>>> under o.a.f.migration, so that at least the flip-6 code is covered.
> >>>>>>
> >>>>>> Similarly, enforcing checkstyle for all tests should be fine as
> well.
> >>>>>>
> >>>>>> Regards,
> >>>>>> Chesnay
> >>>>>>
> >>>>>>
> >>>>>> On 22.02.2017 11:48, Till Rohrmann wrote:
> >>>>>>
> >>>>>> I think that not enforcing a code style is as good as not having any
> >>>>>>>
> >>>>>> code
> >>>>>
> >>>>>> style to be honest. Having an IntelliJ or Eclipse profile is nice
> and
> >>>>>>>
> >>>>>> some
> >>>>>
> >>>>>> people will probably use it, but my gut feeling is that the majority
> >>>>>>>
> >>>>>> won't
> >>>>>
> >>>>>> notice it.
> >>>>>>>
> >>>>>>> Cheers,
> >>>>>>> Till
> >>>>>>>
> >>>>>>> On Wed, Feb 22, 2017 at 11:15 AM, Ufuk Celebi <[hidden email]>
> >> wrote:
> >>>>>>>
> >>>>>>> Kurt's proposal sounds reasonable.
> >>>>>>>
> >>>>>>>> What about the following:
> >>>>>>>> - We try to capture the current style in an code style
> configuration
> >>>>>>>> (for IntelliJ and maybe Eclipse)
> >>>>>>>> - We provide that on the website for contributors to download
> >>>>>>>> - We don't enforce it, but new contributions and changes are free
> to
> >>>>>>>> format with this style as changes happen
> >>>>>>>>
> >>>>>>>> Practically speaking, this should not change much except maybe the
> >>>>>>>> import order or whitespace after certain keywords, etc.
> >>>>>>>>
> >>>>>>>>
> >>>>>>>> On Wed, Feb 22, 2017 at 4:48 AM, Kurt Young <[hidden email]>
> >> wrote:
> >>>>>>>>
> >>>>>>>> +1 to provide a unified code style for both java and scala.
> >>>>>>>>>
> >>>>>>>>> -1 to adjust all the existing code to the new style in one step.
> >> The
> >>>>>>>>>
> >>>>>>>>> code's
> >>>>>>>>
> >>>>>>>> history contains very helpful information which can help
> >>>>>>>>> develops to understand why these codes are added, which jira is
> >>>>>>>>>
> >>>>>>>> related.
> >>>>>
> >>>>>> This information is too valuable to lose. I think we can
> >>>>>>>>> do the reformat thing step by step, each time when the codes
> being
> >>>>>>>>>
> >>>>>>>>> changed,
> >>>>>>>>
> >>>>>>>> we can adopt them to the new style.
> >>>>>>>>> IMHO this is also the reason why the unified code style is
> >> important.
> >>>>>>>>>
> >>>>>>>>>
> >>>>>>>>> Best,
> >>>>>>>>> Kurt
> >>>>>>>>>
> >>>>>>>>> On Wed, Feb 22, 2017 at 5:50 AM, Dawid Wysakowicz <
> >>>>>>>>> [hidden email]> wrote:
> >>>>>>>>>
> >>>>>>>>> Hi,
> >>>>>>>>>
> >>>>>>>>>> I would like to resurrect the discussing ([1]
> >>>>>>>>>> <http://apache-flink-mailing-list-archive.1008284.n3.
> >>>>>>>>>> nabble.com/Code-style-guideline-for-Scala-td7526.html>
> >>>>>>>>>> , [2]
> >>>>>>>>>> <http://apache-flink-mailing-list-archive.1008284.n3.
> >>>>>>>>>> nabble.com/Intellij-code-style-td11092.html>)
> >>>>>>>>>> about creating unified code style(that could be imported to at
> >> least
> >>>>>>>>>> IntelliJ and Eclipse) and corresponding stricter checkstyle
> rules.
> >>>>>>>>>>
> >>>>>>>>>> I know that the hardest part is to adjust the existing code to
> the
> >>>>>>>>>>
> >>>>>>>>> new
> >>>>>
> >>>>>> checkstyle rules. Do you believe it would be worth the effort? All
> >>>>>>>>>> suggestions are welcome.
> >>>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>>
> >>>
> >>
>
>
Reply | Threaded
Open this post in threaded view
|

Re: [DISCUSS] Code style / checkstyle

Ufuk Celebi-2
On Fri, Feb 24, 2017 at 10:46 AM, Fabian Hueske <[hidden email]> wrote:
> I agree with Till that encouraging a code style without enforcing it does
> not make a lot of sense.
> If we enforce it, we need to touch all files and PRs.

I think it makes sense for new contributors to have a starting point
without enforcing anything (I do agree that we are past the point to
reach consensus on a style and enforcement ;-)).
Reply | Threaded
Open this post in threaded view
|

Re: [DISCUSS] Code style / checkstyle

Dawid Wysakowicz
The problem with code style when it is not enforced is that it will be a
matter of luck to what parts of files / new files will it be applied. When
the code style is not applied to whole file, it is pretty much useless
anyway. You would need to manually select just the fragments one is
changing. The benefits of having code style and enforcing it I see are:
 - being able to apply autoformatter, which speeds up writing code
 - it would make reviewing PRs easier as e.g. there would be line length
limit applied etc. which will make line breaking more reader friendly.

Though I think if a consensus is not reachable it would be good to once and
forever decide that we don't want a code style and checkstyle.

2017-02-24 10:51 GMT+01:00 Ufuk Celebi <[hidden email]>:

> On Fri, Feb 24, 2017 at 10:46 AM, Fabian Hueske <[hidden email]> wrote:
> > I agree with Till that encouraging a code style without enforcing it does
> > not make a lot of sense.
> > If we enforce it, we need to touch all files and PRs.
>
> I think it makes sense for new contributors to have a starting point
> without enforcing anything (I do agree that we are past the point to
> reach consensus on a style and enforcement ;-)).
>
Reply | Threaded
Open this post in threaded view
|

Re: [DISCUSS] Code style / checkstyle

Greg Hogan
I agree wholeheartedly with Ufuk. We cannot reformat the codebase, cannot
pause while flushing the PR queue, and won't find a consensus code style.

I think we can create a baseline code style for new and existing
contributors for which reformatting on changed files will be acceptable for
PR reviews.

On Fri, Feb 24, 2017 at 5:01 AM, Dawid Wysakowicz <
[hidden email]> wrote:

> The problem with code style when it is not enforced is that it will be a
> matter of luck to what parts of files / new files will it be applied. When
> the code style is not applied to whole file, it is pretty much useless
> anyway. You would need to manually select just the fragments one is
> changing. The benefits of having code style and enforcing it I see are:
>  - being able to apply autoformatter, which speeds up writing code
>  - it would make reviewing PRs easier as e.g. there would be line length
> limit applied etc. which will make line breaking more reader friendly.
>
> Though I think if a consensus is not reachable it would be good to once and
> forever decide that we don't want a code style and checkstyle.
>
> 2017-02-24 10:51 GMT+01:00 Ufuk Celebi <[hidden email]>:
>
> > On Fri, Feb 24, 2017 at 10:46 AM, Fabian Hueske <[hidden email]>
> wrote:
> > > I agree with Till that encouraging a code style without enforcing it
> does
> > > not make a lot of sense.
> > > If we enforce it, we need to touch all files and PRs.
> >
> > I think it makes sense for new contributors to have a starting point
> > without enforcing anything (I do agree that we are past the point to
> > reach consensus on a style and enforcement ;-)).
> >
>
Reply | Threaded
Open this post in threaded view
|

Re: [DISCUSS] Code style / checkstyle

Henry Saputra
Just want to clarify what unify code style here.

Is the intention to have IDE and Maven plugins to have the same check style
rules?

Or are we talking about having ONE code style for both Java and Scala?

- Henry

On Fri, Feb 24, 2017 at 8:08 AM, Greg Hogan <[hidden email]> wrote:

> I agree wholeheartedly with Ufuk. We cannot reformat the codebase, cannot
> pause while flushing the PR queue, and won't find a consensus code style.
>
> I think we can create a baseline code style for new and existing
> contributors for which reformatting on changed files will be acceptable for
> PR reviews.
>
> On Fri, Feb 24, 2017 at 5:01 AM, Dawid Wysakowicz <
> [hidden email]> wrote:
>
> > The problem with code style when it is not enforced is that it will be a
> > matter of luck to what parts of files / new files will it be applied.
> When
> > the code style is not applied to whole file, it is pretty much useless
> > anyway. You would need to manually select just the fragments one is
> > changing. The benefits of having code style and enforcing it I see are:
> >  - being able to apply autoformatter, which speeds up writing code
> >  - it would make reviewing PRs easier as e.g. there would be line length
> > limit applied etc. which will make line breaking more reader friendly.
> >
> > Though I think if a consensus is not reachable it would be good to once
> and
> > forever decide that we don't want a code style and checkstyle.
> >
> > 2017-02-24 10:51 GMT+01:00 Ufuk Celebi <[hidden email]>:
> >
> > > On Fri, Feb 24, 2017 at 10:46 AM, Fabian Hueske <[hidden email]>
> > wrote:
> > > > I agree with Till that encouraging a code style without enforcing it
> > does
> > > > not make a lot of sense.
> > > > If we enforce it, we need to touch all files and PRs.
> > >
> > > I think it makes sense for new contributors to have a starting point
> > > without enforcing anything (I do agree that we are past the point to
> > > reach consensus on a style and enforcement ;-)).
> > >
> >
>
Reply | Threaded
Open this post in threaded view
|

Re: [DISCUSS] Code style / checkstyle

aalexandrov
Just to clarify - by "losing the commit history" you actually mean "losing
the ability to annotate each line in a file with its last commit", right?

Or is there some other sense in which something is lost after applying bulk
re-format?

Cheers,
A.

On Sat, Feb 25, 2017 at 7:10 AM Henry Saputra <[hidden email]>
wrote:

> Just want to clarify what unify code style here.
>
> Is the intention to have IDE and Maven plugins to have the same check style
> rules?
>
> Or are we talking about having ONE code style for both Java and Scala?
>
> - Henry
>
> On Fri, Feb 24, 2017 at 8:08 AM, Greg Hogan <[hidden email]> wrote:
>
> > I agree wholeheartedly with Ufuk. We cannot reformat the codebase, cannot
> > pause while flushing the PR queue, and won't find a consensus code style.
> >
> > I think we can create a baseline code style for new and existing
> > contributors for which reformatting on changed files will be acceptable
> for
> > PR reviews.
> >
> > On Fri, Feb 24, 2017 at 5:01 AM, Dawid Wysakowicz <
> > [hidden email]> wrote:
> >
> > > The problem with code style when it is not enforced is that it will be
> a
> > > matter of luck to what parts of files / new files will it be applied.
> > When
> > > the code style is not applied to whole file, it is pretty much useless
> > > anyway. You would need to manually select just the fragments one is
> > > changing. The benefits of having code style and enforcing it I see are:
> > >  - being able to apply autoformatter, which speeds up writing code
> > >  - it would make reviewing PRs easier as e.g. there would be line
> length
> > > limit applied etc. which will make line breaking more reader friendly.
> > >
> > > Though I think if a consensus is not reachable it would be good to once
> > and
> > > forever decide that we don't want a code style and checkstyle.
> > >
> > > 2017-02-24 10:51 GMT+01:00 Ufuk Celebi <[hidden email]>:
> > >
> > > > On Fri, Feb 24, 2017 at 10:46 AM, Fabian Hueske <[hidden email]>
> > > wrote:
> > > > > I agree with Till that encouraging a code style without enforcing
> it
> > > does
> > > > > not make a lot of sense.
> > > > > If we enforce it, we need to touch all files and PRs.
> > > >
> > > > I think it makes sense for new contributors to have a starting point
> > > > without enforcing anything (I do agree that we are past the point to
> > > > reach consensus on a style and enforcement ;-)).
> > > >
> > >
> >
>
Reply | Threaded
Open this post in threaded view
|

Re: [DISCUSS] Code style / checkstyle

Till Rohrmann
No, I think that's exactly what people mean when saying "losing the commit
history". With the reformatting you would have to go manually through all
past commits until you find the commit which changed a given line before
the reformatting.

Cheers,
Till

On Sun, Feb 26, 2017 at 6:32 PM, Alexander Alexandrov <
[hidden email]> wrote:

> Just to clarify - by "losing the commit history" you actually mean "losing
> the ability to annotate each line in a file with its last commit", right?
>
> Or is there some other sense in which something is lost after applying bulk
> re-format?
>
> Cheers,
> A.
>
> On Sat, Feb 25, 2017 at 7:10 AM Henry Saputra <[hidden email]>
> wrote:
>
> > Just want to clarify what unify code style here.
> >
> > Is the intention to have IDE and Maven plugins to have the same check
> style
> > rules?
> >
> > Or are we talking about having ONE code style for both Java and Scala?
> >
> > - Henry
> >
> > On Fri, Feb 24, 2017 at 8:08 AM, Greg Hogan <[hidden email]> wrote:
> >
> > > I agree wholeheartedly with Ufuk. We cannot reformat the codebase,
> cannot
> > > pause while flushing the PR queue, and won't find a consensus code
> style.
> > >
> > > I think we can create a baseline code style for new and existing
> > > contributors for which reformatting on changed files will be acceptable
> > for
> > > PR reviews.
> > >
> > > On Fri, Feb 24, 2017 at 5:01 AM, Dawid Wysakowicz <
> > > [hidden email]> wrote:
> > >
> > > > The problem with code style when it is not enforced is that it will
> be
> > a
> > > > matter of luck to what parts of files / new files will it be applied.
> > > When
> > > > the code style is not applied to whole file, it is pretty much
> useless
> > > > anyway. You would need to manually select just the fragments one is
> > > > changing. The benefits of having code style and enforcing it I see
> are:
> > > >  - being able to apply autoformatter, which speeds up writing code
> > > >  - it would make reviewing PRs easier as e.g. there would be line
> > length
> > > > limit applied etc. which will make line breaking more reader
> friendly.
> > > >
> > > > Though I think if a consensus is not reachable it would be good to
> once
> > > and
> > > > forever decide that we don't want a code style and checkstyle.
> > > >
> > > > 2017-02-24 10:51 GMT+01:00 Ufuk Celebi <[hidden email]>:
> > > >
> > > > > On Fri, Feb 24, 2017 at 10:46 AM, Fabian Hueske <[hidden email]
> >
> > > > wrote:
> > > > > > I agree with Till that encouraging a code style without enforcing
> > it
> > > > does
> > > > > > not make a lot of sense.
> > > > > > If we enforce it, we need to touch all files and PRs.
> > > > >
> > > > > I think it makes sense for new contributors to have a starting
> point
> > > > > without enforcing anything (I do agree that we are past the point
> to
> > > > > reach consensus on a style and enforcement ;-)).
> > > > >
> > > >
> > >
> >
>
Reply | Threaded
Open this post in threaded view
|

Re: [DISCUSS] Code style / checkstyle

Aljoscha Krettek-2
Just for a bit of context, this is the output of running cloc on the Flink
codebase:
-----------------------------------------------------------------------------------
Language                         files          blank        comment
    code
-----------------------------------------------------------------------------------
Java                              4609         126825         185428
  519096

=> 704,524 lines of code + comments/javadoc

When I apply the google style to the Flink code base using
https://github.com/google/google-java-format I get these commit statistics:

4577 files changed, 647645 insertions(+), 622663 deletions(-)

That is, a change to the Google Code Style would touch roughly over 90% of
all code/comment lines.

I would like to have a well defined code style, such as the Google Code
style, that has nice tooling and support but I don't think we will ever
convince enough people to do this kind of massive change. Even I think it's
a bit crazy to change 90% of the code base in one commit.

On Mon, 27 Feb 2017 at 11:10 Till Rohrmann <[hidden email]> wrote:

> No, I think that's exactly what people mean when saying "losing the commit
> history". With the reformatting you would have to go manually through all
> past commits until you find the commit which changed a given line before
> the reformatting.
>
> Cheers,
> Till
>
> On Sun, Feb 26, 2017 at 6:32 PM, Alexander Alexandrov <
> [hidden email]> wrote:
>
> > Just to clarify - by "losing the commit history" you actually mean
> "losing
> > the ability to annotate each line in a file with its last commit", right?
> >
> > Or is there some other sense in which something is lost after applying
> bulk
> > re-format?
> >
> > Cheers,
> > A.
> >
> > On Sat, Feb 25, 2017 at 7:10 AM Henry Saputra <[hidden email]>
> > wrote:
> >
> > > Just want to clarify what unify code style here.
> > >
> > > Is the intention to have IDE and Maven plugins to have the same check
> > style
> > > rules?
> > >
> > > Or are we talking about having ONE code style for both Java and Scala?
> > >
> > > - Henry
> > >
> > > On Fri, Feb 24, 2017 at 8:08 AM, Greg Hogan <[hidden email]>
> wrote:
> > >
> > > > I agree wholeheartedly with Ufuk. We cannot reformat the codebase,
> > cannot
> > > > pause while flushing the PR queue, and won't find a consensus code
> > style.
> > > >
> > > > I think we can create a baseline code style for new and existing
> > > > contributors for which reformatting on changed files will be
> acceptable
> > > for
> > > > PR reviews.
> > > >
> > > > On Fri, Feb 24, 2017 at 5:01 AM, Dawid Wysakowicz <
> > > > [hidden email]> wrote:
> > > >
> > > > > The problem with code style when it is not enforced is that it will
> > be
> > > a
> > > > > matter of luck to what parts of files / new files will it be
> applied.
> > > > When
> > > > > the code style is not applied to whole file, it is pretty much
> > useless
> > > > > anyway. You would need to manually select just the fragments one is
> > > > > changing. The benefits of having code style and enforcing it I see
> > are:
> > > > >  - being able to apply autoformatter, which speeds up writing code
> > > > >  - it would make reviewing PRs easier as e.g. there would be line
> > > length
> > > > > limit applied etc. which will make line breaking more reader
> > friendly.
> > > > >
> > > > > Though I think if a consensus is not reachable it would be good to
> > once
> > > > and
> > > > > forever decide that we don't want a code style and checkstyle.
> > > > >
> > > > > 2017-02-24 10:51 GMT+01:00 Ufuk Celebi <[hidden email]>:
> > > > >
> > > > > > On Fri, Feb 24, 2017 at 10:46 AM, Fabian Hueske <
> [hidden email]
> > >
> > > > > wrote:
> > > > > > > I agree with Till that encouraging a code style without
> enforcing
> > > it
> > > > > does
> > > > > > > not make a lot of sense.
> > > > > > > If we enforce it, we need to touch all files and PRs.
> > > > > >
> > > > > > I think it makes sense for new contributors to have a starting
> > point
> > > > > > without enforcing anything (I do agree that we are past the point
> > to
> > > > > > reach consensus on a style and enforcement ;-)).
> > > > > >
> > > > >
> > > >
> > >
> >
>
Reply | Threaded
Open this post in threaded view
|

Re: [DISCUSS] Code style / checkstyle

Stephan Ewen
I agree, reformatting 90% of the code base is tough.

There are two main issues:
  (1) Incompatible merges. This is hard, especially for the folks that have
to merge the pull requests ;-)

  (2) Author history: This is less of an issue, I think. "git log
<filename>" and "git show <revision> -- <filename>" will still work and one
may have to go one commit back to find out why something was changed


What I could image is to do this incrementally. Define the code style in
"flink-parent" but do not activate it.
Then start with some projects (new projects, plus some others):
merge/reject PRs, reformat, activate code style.

Piece by piece. This is realistically going to take a long time until it is
pulled through all components, but that's okay, I guess.

Stephan


On Mon, Feb 27, 2017 at 1:53 PM, Aljoscha Krettek <[hidden email]>
wrote:

> Just for a bit of context, this is the output of running cloc on the Flink
> codebase:
> ------------------------------------------------------------
> -----------------------
> Language                         files          blank        comment
>     code
> ------------------------------------------------------------
> -----------------------
> Java                              4609         126825         185428
>   519096
>
> => 704,524 lines of code + comments/javadoc
>
> When I apply the google style to the Flink code base using
> https://github.com/google/google-java-format I get these commit
> statistics:
>
> 4577 files changed, 647645 insertions(+), 622663 deletions(-)
>
> That is, a change to the Google Code Style would touch roughly over 90% of
> all code/comment lines.
>
> I would like to have a well defined code style, such as the Google Code
> style, that has nice tooling and support but I don't think we will ever
> convince enough people to do this kind of massive change. Even I think it's
> a bit crazy to change 90% of the code base in one commit.
>
> On Mon, 27 Feb 2017 at 11:10 Till Rohrmann <[hidden email]> wrote:
>
> > No, I think that's exactly what people mean when saying "losing the
> commit
> > history". With the reformatting you would have to go manually through all
> > past commits until you find the commit which changed a given line before
> > the reformatting.
> >
> > Cheers,
> > Till
> >
> > On Sun, Feb 26, 2017 at 6:32 PM, Alexander Alexandrov <
> > [hidden email]> wrote:
> >
> > > Just to clarify - by "losing the commit history" you actually mean
> > "losing
> > > the ability to annotate each line in a file with its last commit",
> right?
> > >
> > > Or is there some other sense in which something is lost after applying
> > bulk
> > > re-format?
> > >
> > > Cheers,
> > > A.
> > >
> > > On Sat, Feb 25, 2017 at 7:10 AM Henry Saputra <[hidden email]
> >
> > > wrote:
> > >
> > > > Just want to clarify what unify code style here.
> > > >
> > > > Is the intention to have IDE and Maven plugins to have the same check
> > > style
> > > > rules?
> > > >
> > > > Or are we talking about having ONE code style for both Java and
> Scala?
> > > >
> > > > - Henry
> > > >
> > > > On Fri, Feb 24, 2017 at 8:08 AM, Greg Hogan <[hidden email]>
> > wrote:
> > > >
> > > > > I agree wholeheartedly with Ufuk. We cannot reformat the codebase,
> > > cannot
> > > > > pause while flushing the PR queue, and won't find a consensus code
> > > style.
> > > > >
> > > > > I think we can create a baseline code style for new and existing
> > > > > contributors for which reformatting on changed files will be
> > acceptable
> > > > for
> > > > > PR reviews.
> > > > >
> > > > > On Fri, Feb 24, 2017 at 5:01 AM, Dawid Wysakowicz <
> > > > > [hidden email]> wrote:
> > > > >
> > > > > > The problem with code style when it is not enforced is that it
> will
> > > be
> > > > a
> > > > > > matter of luck to what parts of files / new files will it be
> > applied.
> > > > > When
> > > > > > the code style is not applied to whole file, it is pretty much
> > > useless
> > > > > > anyway. You would need to manually select just the fragments one
> is
> > > > > > changing. The benefits of having code style and enforcing it I
> see
> > > are:
> > > > > >  - being able to apply autoformatter, which speeds up writing
> code
> > > > > >  - it would make reviewing PRs easier as e.g. there would be line
> > > > length
> > > > > > limit applied etc. which will make line breaking more reader
> > > friendly.
> > > > > >
> > > > > > Though I think if a consensus is not reachable it would be good
> to
> > > once
> > > > > and
> > > > > > forever decide that we don't want a code style and checkstyle.
> > > > > >
> > > > > > 2017-02-24 10:51 GMT+01:00 Ufuk Celebi <[hidden email]>:
> > > > > >
> > > > > > > On Fri, Feb 24, 2017 at 10:46 AM, Fabian Hueske <
> > [hidden email]
> > > >
> > > > > > wrote:
> > > > > > > > I agree with Till that encouraging a code style without
> > enforcing
> > > > it
> > > > > > does
> > > > > > > > not make a lot of sense.
> > > > > > > > If we enforce it, we need to touch all files and PRs.
> > > > > > >
> > > > > > > I think it makes sense for new contributors to have a starting
> > > point
> > > > > > > without enforcing anything (I do agree that we are past the
> point
> > > to
> > > > > > > reach consensus on a style and enforcement ;-)).
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
12