[DISCUSS] Enforce common opinionated coding style using Spotless

[Aljoscha Krettek-2]

Hi All,

I know I know, but please keep reading because I recently learned about
some new developments in the area of coding-style automation.

The tool I would propose we use is Spotless
(https://github.com/diffplug/spotless). This doesn't come with a
formatter but allows using other popular formatters such as
google-java-format. The nice thing about Spotless is that it serves as a
verifier for CI but can also apply the configured style automatically.
That is, for the programmer all she has to do is `mvn spotless:apply` to
fix any style violations.

An interesting feature, which was (somewhat) recently added is "ratchet"
(https://github.com/diffplug/spotless/blob/main/plugin-maven/README.md#ratchet).
With this, you can set up Spotless to only apply it's rules to files
that were changed after a configured commit. This would allow a gradual
application of the new coding style instead of one big change.

If we decide to use Spotless, we would of course also have to decide on
a coding style. For this I would propose google-java-format, which the
flink-statefun project uses. The main difference from our current
"style" is that this uses spaces instead of tabs for indentation. By
default it would be 2 spaces but it can be configured to use 4 spaces
which would make code look more or less like our current style. There
are no more configuration knobs, so using tabs is not an option.

Finally, why should we do this? I think most engineers agree that having
a common enforced style is good to have so I only want to highlight a
few thoughts here about things we could improve:

  - No more nits about coding style in reviews, this makes it easier for
both the reviewer and developer

  - No manual fixing of Checkstyle errors because Spotless can do that
automatically

  - Because Flink is such a big project little islands of coding style
have formed between people that commonly work on components. It can be a
nuisance when you work on a different component and then reviewers don't
like your typical coding style. And you first have to get used to the
slight differences in style when reading code.

There are also downsides I see in this:

  - We break the history, but both "git blame" and modern IntelliJ can
ignore whitespace when attributing changes. So for files that are
already "well" formatted not much would change.

  - In the short-term it will be harder to apply changes both to master
and one of the release-x branches because formatting will be different.
I think this is not too hard though because Spotless can automatically
apply the style.

In summary, we would have some short-term pain with this but I think it
would be good in the long run. What are your thoughts?

Best,
Aljoscha

[Flavio Pompermaier]

Hi Aljoscha,
I think that having the style check directly in the IDE is a very good
feature so +1 on my side as a contributor (I also asked once on the mailing
list if there was already something like that)..I never used Spotless so I
can't say if it easy to integrate with the IDE but the nice thing is that
is has plugins both for Eclipse and IntelliJ so it was already on my
wish-to-try list ;)

Bye,
Flavio

On Tue, Oct 6, 2020 at 2:15 PM Aljoscha Krettek <[hidden email]> wrote:

> Hi All,
>
> I know I know, but please keep reading because I recently learned about
> some new developments in the area of coding-style automation.
>
> The tool I would propose we use is Spotless
> (https://github.com/diffplug/spotless). This doesn't come with a
> formatter but allows using other popular formatters such as
> google-java-format. The nice thing about Spotless is that it serves as a
> verifier for CI but can also apply the configured style automatically.
> That is, for the programmer all she has to do is `mvn spotless:apply` to
> fix any style violations.
>
> An interesting feature, which was (somewhat) recently added is "ratchet"
> (
> https://github.com/diffplug/spotless/blob/main/plugin-maven/README.md#ratchet).
>
> With this, you can set up Spotless to only apply it's rules to files
> that were changed after a configured commit. This would allow a gradual
> application of the new coding style instead of one big change.
>
> If we decide to use Spotless, we would of course also have to decide on
> a coding style. For this I would propose google-java-format, which the
> flink-statefun project uses. The main difference from our current
> "style" is that this uses spaces instead of tabs for indentation. By
> default it would be 2 spaces but it can be configured to use 4 spaces
> which would make code look more or less like our current style. There
> are no more configuration knobs, so using tabs is not an option.
>
> Finally, why should we do this? I think most engineers agree that having
> a common enforced style is good to have so I only want to highlight a
> few thoughts here about things we could improve:
>
>   - No more nits about coding style in reviews, this makes it easier for
> both the reviewer and developer
>
>   - No manual fixing of Checkstyle errors because Spotless can do that
> automatically
>
>   - Because Flink is such a big project little islands of coding style
> have formed between people that commonly work on components. It can be a
> nuisance when you work on a different component and then reviewers don't
> like your typical coding style. And you first have to get used to the
> slight differences in style when reading code.
>
> There are also downsides I see in this:
>
>   - We break the history, but both "git blame" and modern IntelliJ can
> ignore whitespace when attributing changes. So for files that are
> already "well" formatted not much would change.
>
>   - In the short-term it will be harder to apply changes both to master
> and one of the release-x branches because formatting will be different.
> I think this is not too hard though because Spotless can automatically
> apply the style.
>
> In summary, we would have some short-term pain with this but I think it
> would be good in the long run. What are your thoughts?
>
> Best,
> Aljoscha

[Chesnay Schepler-3]

We shouldn't switch to spaces _now_; cutting this bit from your proposal
will massively simplify things and there's hardly any value in changing it.

Also I'm getting rather tired of this constant idea of "gradual
application". We've been doing this for 2-3 years now since we
introduced Checkstyle and basically got nowhere. We should just bite the
bullet and get it over with; we could've solved this whole problem already.

In conclusion, I'm +1 on finally locking down the codestyle and applying
it immediately, I'm -1 on any gradual application scheme because they
_just don't work_.

On 10/6/2020 2:15 PM, Aljoscha Krettek wrote:

> Hi All,
>
> I know I know, but please keep reading because I recently learned
> about some new developments in the area of coding-style automation.
>
> The tool I would propose we use is Spotless
> (https://github.com/diffplug/spotless). This doesn't come with a
> formatter but allows using other popular formatters such as
> google-java-format. The nice thing about Spotless is that it serves as
> a verifier for CI but can also apply the configured style
> automatically. That is, for the programmer all she has to do is `mvn
> spotless:apply` to fix any style violations.
>
> An interesting feature, which was (somewhat) recently added is
> "ratchet"
> (https://github.com/diffplug/spotless/blob/main/plugin-maven/README.md#ratchet).
> With this, you can set up Spotless to only apply it's rules to files
> that were changed after a configured commit. This would allow a
> gradual application of the new coding style instead of one big change.
>
> If we decide to use Spotless, we would of course also have to decide
> on a coding style. For this I would propose google-java-format, which
> the flink-statefun project uses. The main difference from our current
> "style" is that this uses spaces instead of tabs for indentation. By
> default it would be 2 spaces but it can be configured to use 4 spaces
> which would make code look more or less like our current style. There
> are no more configuration knobs, so using tabs is not an option.
>
> Finally, why should we do this? I think most engineers agree that
> having a common enforced style is good to have so I only want to
> highlight a few thoughts here about things we could improve:
>
>  - No more nits about coding style in reviews, this makes it easier
> for both the reviewer and developer
>
>  - No manual fixing of Checkstyle errors because Spotless can do that
> automatically
>
>  - Because Flink is such a big project little islands of coding style
> have formed between people that commonly work on components. It can be
> a nuisance when you work on a different component and then reviewers
> don't like your typical coding style. And you first have to get used
> to the slight differences in style when reading code.
>
> There are also downsides I see in this:
>
>  - We break the history, but both "git blame" and modern IntelliJ can
> ignore whitespace when attributing changes. So for files that are
> already "well" formatted not much would change.
>
>  - In the short-term it will be harder to apply changes both to master
> and one of the release-x branches because formatting will be
> different. I think this is not too hard though because Spotless can
> automatically apply the style.
>
> In summary, we would have some short-term pain with this but I think
> it would be good in the long run. What are your thoughts?
>
> Best,
> Aljoscha
>

[Arvid Heise-3]

I'm also +1 for automatically enforceable code style.

I also would just go over it as Chesnay said. While it makes some changes a
bit harder to track (inline git blame), it's easy to skip over in any git
history and if it's only one massive commit, then it's also much easier to
ignore than many gradual changes. Further, if we just do it once, git blame
will quickly become more reliable again.

Btw I completely don't care about the code style as long as it plays well
with IntelliJ (it used to be different, but things change :p).

On Tue, Oct 6, 2020 at 5:23 PM Chesnay Schepler <[hidden email]> wrote:

> We shouldn't switch to spaces _now_; cutting this bit from your proposal
> will massively simplify things and there's hardly any value in changing it.
>
> Also I'm getting rather tired of this constant idea of "gradual
> application". We've been doing this for 2-3 years now since we
> introduced Checkstyle and basically got nowhere. We should just bite the
> bullet and get it over with; we could've solved this whole problem already.
>
> In conclusion, I'm +1 on finally locking down the codestyle and applying
> it immediately, I'm -1 on any gradual application scheme because they
> _just don't work_.
>
> On 10/6/2020 2:15 PM, Aljoscha Krettek wrote:
> > Hi All,
> >
> > I know I know, but please keep reading because I recently learned
> > about some new developments in the area of coding-style automation.
> >
> > The tool I would propose we use is Spotless
> > (https://github.com/diffplug/spotless). This doesn't come with a
> > formatter but allows using other popular formatters such as
> > google-java-format. The nice thing about Spotless is that it serves as
> > a verifier for CI but can also apply the configured style
> > automatically. That is, for the programmer all she has to do is `mvn
> > spotless:apply` to fix any style violations.
> >
> > An interesting feature, which was (somewhat) recently added is
> > "ratchet"
> > (
> https://github.com/diffplug/spotless/blob/main/plugin-maven/README.md#ratchet).
>
> > With this, you can set up Spotless to only apply it's rules to files
> > that were changed after a configured commit. This would allow a
> > gradual application of the new coding style instead of one big change.
> >
> > If we decide to use Spotless, we would of course also have to decide
> > on a coding style. For this I would propose google-java-format, which
> > the flink-statefun project uses. The main difference from our current
> > "style" is that this uses spaces instead of tabs for indentation. By
> > default it would be 2 spaces but it can be configured to use 4 spaces
> > which would make code look more or less like our current style. There
> > are no more configuration knobs, so using tabs is not an option.
> >
> > Finally, why should we do this? I think most engineers agree that
> > having a common enforced style is good to have so I only want to
> > highlight a few thoughts here about things we could improve:
> >
> >  - No more nits about coding style in reviews, this makes it easier
> > for both the reviewer and developer
> >
> >  - No manual fixing of Checkstyle errors because Spotless can do that
> > automatically
> >
> >  - Because Flink is such a big project little islands of coding style
> > have formed between people that commonly work on components. It can be
> > a nuisance when you work on a different component and then reviewers
> > don't like your typical coding style. And you first have to get used
> > to the slight differences in style when reading code.
> >
> > There are also downsides I see in this:
> >
> >  - We break the history, but both "git blame" and modern IntelliJ can
> > ignore whitespace when attributing changes. So for files that are
> > already "well" formatted not much would change.
> >
> >  - In the short-term it will be harder to apply changes both to master
> > and one of the release-x branches because formatting will be
> > different. I think this is not too hard though because Spotless can
> > automatically apply the style.
> >
> > In summary, we would have some short-term pain with this but I think
> > it would be good in the long run. What are your thoughts?
> >
> > Best,
> > Aljoscha
> >
>
>

--

Arvid Heise | Senior Java Developer

<https://www.ververica.com/>

Follow us @VervericaData

--

Join Flink Forward <https://flink-forward.org/> - The Apache Flink
Conference

Stream Processing | Event Driven | Real Time

--

Ververica GmbH | Invalidenstrasse 115, 10115 Berlin, Germany

--
Ververica GmbH
Registered at Amtsgericht Charlottenburg: HRB 158244 B
Managing Directors: Timothy Alexander Steinert, Yip Park Tung Jason, Ji
(Toni) Cheng

[Arvid Heise-3]

After having written that I did a quick search, you can even use git blame
with one big massive change commit [1], which would further help the idea
of "just get over with it".

With that option, I'd even change all whitespaces if the community thinks
that it's a better option (a separate discussion that I'll gladly skip).

[1]
https://www.moxio.com/blog/43/ignoring-bulk-change-commits-with-git-blame

On Tue, Oct 6, 2020 at 5:38 PM Arvid Heise <[hidden email]> wrote:

> I'm also +1 for automatically enforceable code style.
>
> I also would just go over it as Chesnay said. While it makes some changes
> a bit harder to track (inline git blame), it's easy to skip over in any git
> history and if it's only one massive commit, then it's also much easier to
> ignore than many gradual changes. Further, if we just do it once, git blame
> will quickly become more reliable again.
>
> Btw I completely don't care about the code style as long as it plays well
> with IntelliJ (it used to be different, but things change :p).
>
> On Tue, Oct 6, 2020 at 5:23 PM Chesnay Schepler <[hidden email]>
> wrote:
>
>> We shouldn't switch to spaces _now_; cutting this bit from your proposal
>> will massively simplify things and there's hardly any value in changing
>> it.
>>
>> Also I'm getting rather tired of this constant idea of "gradual
>> application". We've been doing this for 2-3 years now since we
>> introduced Checkstyle and basically got nowhere. We should just bite the
>> bullet and get it over with; we could've solved this whole problem
>> already.
>>
>> In conclusion, I'm +1 on finally locking down the codestyle and applying
>> it immediately, I'm -1 on any gradual application scheme because they
>> _just don't work_.
>>
>> On 10/6/2020 2:15 PM, Aljoscha Krettek wrote:
>> > Hi All,
>> >
>> > I know I know, but please keep reading because I recently learned
>> > about some new developments in the area of coding-style automation.
>> >
>> > The tool I would propose we use is Spotless
>> > (https://github.com/diffplug/spotless). This doesn't come with a
>> > formatter but allows using other popular formatters such as
>> > google-java-format. The nice thing about Spotless is that it serves as
>> > a verifier for CI but can also apply the configured style
>> > automatically. That is, for the programmer all she has to do is `mvn
>> > spotless:apply` to fix any style violations.
>> >
>> > An interesting feature, which was (somewhat) recently added is
>> > "ratchet"
>> > (
>> https://github.com/diffplug/spotless/blob/main/plugin-maven/README.md#ratchet).
>>
>> > With this, you can set up Spotless to only apply it's rules to files
>> > that were changed after a configured commit. This would allow a
>> > gradual application of the new coding style instead of one big change.
>> >
>> > If we decide to use Spotless, we would of course also have to decide
>> > on a coding style. For this I would propose google-java-format, which
>> > the flink-statefun project uses. The main difference from our current
>> > "style" is that this uses spaces instead of tabs for indentation. By
>> > default it would be 2 spaces but it can be configured to use 4 spaces
>> > which would make code look more or less like our current style. There
>> > are no more configuration knobs, so using tabs is not an option.
>> >
>> > Finally, why should we do this? I think most engineers agree that
>> > having a common enforced style is good to have so I only want to
>> > highlight a few thoughts here about things we could improve:
>> >
>> >  - No more nits about coding style in reviews, this makes it easier
>> > for both the reviewer and developer
>> >
>> >  - No manual fixing of Checkstyle errors because Spotless can do that
>> > automatically
>> >
>> >  - Because Flink is such a big project little islands of coding style
>> > have formed between people that commonly work on components. It can be
>> > a nuisance when you work on a different component and then reviewers
>> > don't like your typical coding style. And you first have to get used
>> > to the slight differences in style when reading code.
>> >
>> > There are also downsides I see in this:
>> >
>> >  - We break the history, but both "git blame" and modern IntelliJ can
>> > ignore whitespace when attributing changes. So for files that are
>> > already "well" formatted not much would change.
>> >
>> >  - In the short-term it will be harder to apply changes both to master
>> > and one of the release-x branches because formatting will be
>> > different. I think this is not too hard though because Spotless can
>> > automatically apply the style.
>> >
>> > In summary, we would have some short-term pain with this but I think
>> > it would be good in the long run. What are your thoughts?
>> >
>> > Best,
>> > Aljoscha
>> >
>>
>>
>
> --
>
> Arvid Heise | Senior Java Developer
>
> <https://www.ververica.com/>
>
> Follow us @VervericaData
>
> --
>
> Join Flink Forward <https://flink-forward.org/> - The Apache Flink
> Conference
>
> Stream Processing | Event Driven | Real Time
>
> --
>
> Ververica GmbH | Invalidenstrasse 115, 10115 Berlin, Germany
>
> --
> Ververica GmbH
> Registered at Amtsgericht Charlottenburg: HRB 158244 B
> Managing Directors: Timothy Alexander Steinert, Yip Park Tung Jason, Ji
> (Toni) Cheng
>

--

Arvid Heise | Senior Java Developer

<https://www.ververica.com/>

Follow us @VervericaData

--

Join Flink Forward <https://flink-forward.org/> - The Apache Flink
Conference

Stream Processing | Event Driven | Real Time

--

Ververica GmbH | Invalidenstrasse 115, 10115 Berlin, Germany

--
Ververica GmbH
Registered at Amtsgericht Charlottenburg: HRB 158244 B
Managing Directors: Timothy Alexander Steinert, Yip Park Tung Jason, Ji
(Toni) Cheng

[Aljoscha Krettek-2]

Maybe I wasn't very clear on how the ratchet works. The changes are
gradual yes, but it doesn't leave you an option: if you touch a file you
will it will have to conform to the coding style afterwards. It's not
like the previous gradual process we had before where it would be based
on people actively working towards a style.

That being said, I also completely like the option of just doing one big
change commit.

Regarding actual coding styles: we're a bit limited by what tools exist.
I like Spotless because it can be used to both check and apply a style.
Then you need a formatter that works with Spotless and of those we only
have the Eclipse Formatter, google-java-format, and Prettier. Prettier
is a Javascript tool that I would like to avoid. Eclipse is doable but
you need to fiddle with configuration files. I like google-java-format
because of it's take-it-or-leave-it approach. You either use the style
or you don't but it's very thorough. The downside is that it won't do
tabs-only formatting.

Best,
Aljoscha

On 06.10.20 17:43, Arvid Heise wrote:

> After having written that I did a quick search, you can even use git blame
> with one big massive change commit [1], which would further help the idea
> of "just get over with it".
>
> With that option, I'd even change all whitespaces if the community thinks
> that it's a better option (a separate discussion that I'll gladly skip).
>
> [1]
> https://www.moxio.com/blog/43/ignoring-bulk-change-commits-with-git-blame
>
> On Tue, Oct 6, 2020 at 5:38 PM Arvid Heise <[hidden email]> wrote:
>
>> I'm also +1 for automatically enforceable code style.
>>
>> I also would just go over it as Chesnay said. While it makes some changes
>> a bit harder to track (inline git blame), it's easy to skip over in any git
>> history and if it's only one massive commit, then it's also much easier to
>> ignore than many gradual changes. Further, if we just do it once, git blame
>> will quickly become more reliable again.
>>
>> Btw I completely don't care about the code style as long as it plays well
>> with IntelliJ (it used to be different, but things change :p).
>>
>> On Tue, Oct 6, 2020 at 5:23 PM Chesnay Schepler <[hidden email]>
>> wrote:
>>
>>> We shouldn't switch to spaces _now_; cutting this bit from your proposal
>>> will massively simplify things and there's hardly any value in changing
>>> it.
>>>
>>> Also I'm getting rather tired of this constant idea of "gradual
>>> application". We've been doing this for 2-3 years now since we
>>> introduced Checkstyle and basically got nowhere. We should just bite the
>>> bullet and get it over with; we could've solved this whole problem
>>> already.
>>>
>>> In conclusion, I'm +1 on finally locking down the codestyle and applying
>>> it immediately, I'm -1 on any gradual application scheme because they
>>> _just don't work_.
>>>
>>> On 10/6/2020 2:15 PM, Aljoscha Krettek wrote:
>>>> Hi All,
>>>>
>>>> I know I know, but please keep reading because I recently learned
>>>> about some new developments in the area of coding-style automation.
>>>>
>>>> The tool I would propose we use is Spotless
>>>> (https://github.com/diffplug/spotless). This doesn't come with a
>>>> formatter but allows using other popular formatters such as
>>>> google-java-format. The nice thing about Spotless is that it serves as
>>>> a verifier for CI but can also apply the configured style
>>>> automatically. That is, for the programmer all she has to do is `mvn
>>>> spotless:apply` to fix any style violations.
>>>>
>>>> An interesting feature, which was (somewhat) recently added is
>>>> "ratchet"
>>>> (
>>> https://github.com/diffplug/spotless/blob/main/plugin-maven/README.md#ratchet).
>>>
>>>> With this, you can set up Spotless to only apply it's rules to files
>>>> that were changed after a configured commit. This would allow a
>>>> gradual application of the new coding style instead of one big change.
>>>>
>>>> If we decide to use Spotless, we would of course also have to decide
>>>> on a coding style. For this I would propose google-java-format, which
>>>> the flink-statefun project uses. The main difference from our current
>>>> "style" is that this uses spaces instead of tabs for indentation. By
>>>> default it would be 2 spaces but it can be configured to use 4 spaces
>>>> which would make code look more or less like our current style. There
>>>> are no more configuration knobs, so using tabs is not an option.
>>>>
>>>> Finally, why should we do this? I think most engineers agree that
>>>> having a common enforced style is good to have so I only want to
>>>> highlight a few thoughts here about things we could improve:
>>>>
>>>>   - No more nits about coding style in reviews, this makes it easier
>>>> for both the reviewer and developer
>>>>
>>>>   - No manual fixing of Checkstyle errors because Spotless can do that
>>>> automatically
>>>>
>>>>   - Because Flink is such a big project little islands of coding style
>>>> have formed between people that commonly work on components. It can be
>>>> a nuisance when you work on a different component and then reviewers
>>>> don't like your typical coding style. And you first have to get used
>>>> to the slight differences in style when reading code.
>>>>
>>>> There are also downsides I see in this:
>>>>
>>>>   - We break the history, but both "git blame" and modern IntelliJ can
>>>> ignore whitespace when attributing changes. So for files that are
>>>> already "well" formatted not much would change.
>>>>
>>>>   - In the short-term it will be harder to apply changes both to master
>>>> and one of the release-x branches because formatting will be
>>>> different. I think this is not too hard though because Spotless can
>>>> automatically apply the style.
>>>>
>>>> In summary, we would have some short-term pain with this but I think
>>>> it would be good in the long run. What are your thoughts?
>>>>
>>>> Best,
>>>> Aljoscha
>>>>
>>>
>>>
>>
>> --
>>
>> Arvid Heise | Senior Java Developer
>>
>> <https://www.ververica.com/>
>>
>> Follow us @VervericaData
>>
>> --
>>
>> Join Flink Forward <https://flink-forward.org/> - The Apache Flink
>> Conference
>>
>> Stream Processing | Event Driven | Real Time
>>
>> --
>>
>> Ververica GmbH | Invalidenstrasse 115, 10115 Berlin, Germany
>>
>> --
>> Ververica GmbH
>> Registered at Amtsgericht Charlottenburg: HRB 158244 B
>> Managing Directors: Timothy Alexander Steinert, Yip Park Tung Jason, Ji
>> (Toni) Cheng
>>
>
>

[Matthias]

I find the ratchet feature you're suggesting interesting. But Arvid has a
point referring to the blog post about ignoring revisions in git blame [1].
Adding the configuration file for commits to ignore revs as proposed in the
blog post makes it even easier. One problem I see is that this is not
supported by Github (yet?) [2] as mentioned in [1].

Considering all that I prefer applying the code style in one go. I have no
strong opinion on what codestyle is the best.

PS: We used spotless in the project I previously worked on. It was
convenient to use.

[1]
https://www.moxio.com/blog/43/ignoring-bulk-change-commits-with-git-blame
[2]
https://github.community/t/support-ignore-revs-file-in-githubs-blame-view/3256

On Tue, Oct 6, 2020 at 6:00 PM Aljoscha Krettek <[hidden email]> wrote:

> Maybe I wasn't very clear on how the ratchet works. The changes are
> gradual yes, but it doesn't leave you an option: if you touch a file you
> will it will have to conform to the coding style afterwards. It's not
> like the previous gradual process we had before where it would be based
> on people actively working towards a style.
>
> That being said, I also completely like the option of just doing one big
> change commit.
>
> Regarding actual coding styles: we're a bit limited by what tools exist.
> I like Spotless because it can be used to both check and apply a style.
> Then you need a formatter that works with Spotless and of those we only
> have the Eclipse Formatter, google-java-format, and Prettier. Prettier
> is a Javascript tool that I would like to avoid. Eclipse is doable but
> you need to fiddle with configuration files. I like google-java-format
> because of it's take-it-or-leave-it approach. You either use the style
> or you don't but it's very thorough. The downside is that it won't do
> tabs-only formatting.
>
> Best,
> Aljoscha
>
> On 06.10.20 17:43, Arvid Heise wrote:
> > After having written that I did a quick search, you can even use git
> blame
> > with one big massive change commit [1], which would further help the idea
> > of "just get over with it".
> >
> > With that option, I'd even change all whitespaces if the community thinks
> > that it's a better option (a separate discussion that I'll gladly skip).
> >
> > [1]
> >
> https://www.moxio.com/blog/43/ignoring-bulk-change-commits-with-git-blame
> >
> > On Tue, Oct 6, 2020 at 5:38 PM Arvid Heise <[hidden email]> wrote:
> >
> >> I'm also +1 for automatically enforceable code style.
> >>
> >> I also would just go over it as Chesnay said. While it makes some
> changes
> >> a bit harder to track (inline git blame), it's easy to skip over in any
> git
> >> history and if it's only one massive commit, then it's also much easier
> to
> >> ignore than many gradual changes. Further, if we just do it once, git
> blame
> >> will quickly become more reliable again.
> >>
> >> Btw I completely don't care about the code style as long as it plays
> well
> >> with IntelliJ (it used to be different, but things change :p).
> >>
> >> On Tue, Oct 6, 2020 at 5:23 PM Chesnay Schepler <[hidden email]>
> >> wrote:
> >>
> >>> We shouldn't switch to spaces _now_; cutting this bit from your
> proposal
> >>> will massively simplify things and there's hardly any value in changing
> >>> it.
> >>>
> >>> Also I'm getting rather tired of this constant idea of "gradual
> >>> application". We've been doing this for 2-3 years now since we
> >>> introduced Checkstyle and basically got nowhere. We should just bite
> the
> >>> bullet and get it over with; we could've solved this whole problem
> >>> already.
> >>>
> >>> In conclusion, I'm +1 on finally locking down the codestyle and
> applying
> >>> it immediately, I'm -1 on any gradual application scheme because they
> >>> _just don't work_.
> >>>
> >>> On 10/6/2020 2:15 PM, Aljoscha Krettek wrote:
> >>>> Hi All,
> >>>>
> >>>> I know I know, but please keep reading because I recently learned
> >>>> about some new developments in the area of coding-style automation.
> >>>>
> >>>> The tool I would propose we use is Spotless
> >>>> (https://github.com/diffplug/spotless). This doesn't come with a
> >>>> formatter but allows using other popular formatters such as
> >>>> google-java-format. The nice thing about Spotless is that it serves as
> >>>> a verifier for CI but can also apply the configured style
> >>>> automatically. That is, for the programmer all she has to do is `mvn
> >>>> spotless:apply` to fix any style violations.
> >>>>
> >>>> An interesting feature, which was (somewhat) recently added is
> >>>> "ratchet"
> >>>> (
> >>>
> https://github.com/diffplug/spotless/blob/main/plugin-maven/README.md#ratchet
> ).
> >>>
> >>>> With this, you can set up Spotless to only apply it's rules to files
> >>>> that were changed after a configured commit. This would allow a
> >>>> gradual application of the new coding style instead of one big change.
> >>>>
> >>>> If we decide to use Spotless, we would of course also have to decide
> >>>> on a coding style. For this I would propose google-java-format, which
> >>>> the flink-statefun project uses. The main difference from our current
> >>>> "style" is that this uses spaces instead of tabs for indentation. By
> >>>> default it would be 2 spaces but it can be configured to use 4 spaces
> >>>> which would make code look more or less like our current style. There
> >>>> are no more configuration knobs, so using tabs is not an option.
> >>>>
> >>>> Finally, why should we do this? I think most engineers agree that
> >>>> having a common enforced style is good to have so I only want to
> >>>> highlight a few thoughts here about things we could improve:
> >>>>
> >>>>   - No more nits about coding style in reviews, this makes it easier
> >>>> for both the reviewer and developer
> >>>>
> >>>>   - No manual fixing of Checkstyle errors because Spotless can do that
> >>>> automatically
> >>>>
> >>>>   - Because Flink is such a big project little islands of coding style
> >>>> have formed between people that commonly work on components. It can be
> >>>> a nuisance when you work on a different component and then reviewers
> >>>> don't like your typical coding style. And you first have to get used
> >>>> to the slight differences in style when reading code.
> >>>>
> >>>> There are also downsides I see in this:
> >>>>
> >>>>   - We break the history, but both "git blame" and modern IntelliJ can
> >>>> ignore whitespace when attributing changes. So for files that are
> >>>> already "well" formatted not much would change.
> >>>>
> >>>>   - In the short-term it will be harder to apply changes both to
> master
> >>>> and one of the release-x branches because formatting will be
> >>>> different. I think this is not too hard though because Spotless can
> >>>> automatically apply the style.
> >>>>
> >>>> In summary, we would have some short-term pain with this but I think
> >>>> it would be good in the long run. What are your thoughts?
> >>>>
> >>>> Best,
> >>>> Aljoscha
> >>>>
> >>>
> >>>
> >>
> >> --
> >>
> >> Arvid Heise | Senior Java Developer
> >>
> >> <https://www.ververica.com/>
> >>
> >> Follow us @VervericaData
> >>
> >> --
> >>
> >> Join Flink Forward <https://flink-forward.org/> - The Apache Flink
> >> Conference
> >>
> >> Stream Processing | Event Driven | Real Time
> >>
> >> --
> >>
> >> Ververica GmbH | Invalidenstrasse 115, 10115 Berlin, Germany
> >>
> >> --
> >> Ververica GmbH
> >> Registered at Amtsgericht Charlottenburg: HRB 158244 B
> >> Managing Directors: Timothy Alexander Steinert, Yip Park Tung Jason, Ji
> >> (Toni) Cheng
> >>
> >
> >
>

[Chesnay Schepler-3]

To me, ratchet seems to combine the worst aspects of both approaches.
You get disruptive changes, but only in singular files,
for something mundane as a typo fix or import change, which would be
annoying to keep separate from the actual functional changes in a PR.

On 10/7/2020 10:04 AM, Matthias Pohl wrote:

> I find the ratchet feature you're suggesting interesting. But Arvid has a
> point referring to the blog post about ignoring revisions in git blame [1].
> Adding the configuration file for commits to ignore revs as proposed in the
> blog post makes it even easier. One problem I see is that this is not
> supported by Github (yet?) [2] as mentioned in [1].
>
> Considering all that I prefer applying the code style in one go. I have no
> strong opinion on what codestyle is the best.
>
> PS: We used spotless in the project I previously worked on. It was
> convenient to use.
>
> [1]
> https://www.moxio.com/blog/43/ignoring-bulk-change-commits-with-git-blame
> [2]
> https://github.community/t/support-ignore-revs-file-in-githubs-blame-view/3256
>
> On Tue, Oct 6, 2020 at 6:00 PM Aljoscha Krettek <[hidden email]> wrote:
>
>> Maybe I wasn't very clear on how the ratchet works. The changes are
>> gradual yes, but it doesn't leave you an option: if you touch a file you
>> will it will have to conform to the coding style afterwards. It's not
>> like the previous gradual process we had before where it would be based
>> on people actively working towards a style.
>>
>> That being said, I also completely like the option of just doing one big
>> change commit.
>>
>> Regarding actual coding styles: we're a bit limited by what tools exist.
>> I like Spotless because it can be used to both check and apply a style.
>> Then you need a formatter that works with Spotless and of those we only
>> have the Eclipse Formatter, google-java-format, and Prettier. Prettier
>> is a Javascript tool that I would like to avoid. Eclipse is doable but
>> you need to fiddle with configuration files. I like google-java-format
>> because of it's take-it-or-leave-it approach. You either use the style
>> or you don't but it's very thorough. The downside is that it won't do
>> tabs-only formatting.
>>
>> Best,
>> Aljoscha
>>
>> On 06.10.20 17:43, Arvid Heise wrote:
>>> After having written that I did a quick search, you can even use git
>> blame
>>> with one big massive change commit [1], which would further help the idea
>>> of "just get over with it".
>>>
>>> With that option, I'd even change all whitespaces if the community thinks
>>> that it's a better option (a separate discussion that I'll gladly skip).
>>>
>>> [1]
>>>
>> https://www.moxio.com/blog/43/ignoring-bulk-change-commits-with-git-blame
>>> On Tue, Oct 6, 2020 at 5:38 PM Arvid Heise <[hidden email]> wrote:
>>>
>>>> I'm also +1 for automatically enforceable code style.
>>>>
>>>> I also would just go over it as Chesnay said. While it makes some
>> changes
>>>> a bit harder to track (inline git blame), it's easy to skip over in any
>> git
>>>> history and if it's only one massive commit, then it's also much easier
>> to
>>>> ignore than many gradual changes. Further, if we just do it once, git
>> blame
>>>> will quickly become more reliable again.
>>>>
>>>> Btw I completely don't care about the code style as long as it plays
>> well
>>>> with IntelliJ (it used to be different, but things change :p).
>>>>
>>>> On Tue, Oct 6, 2020 at 5:23 PM Chesnay Schepler <[hidden email]>
>>>> wrote:
>>>>
>>>>> We shouldn't switch to spaces _now_; cutting this bit from your
>> proposal
>>>>> will massively simplify things and there's hardly any value in changing
>>>>> it.
>>>>>
>>>>> Also I'm getting rather tired of this constant idea of "gradual
>>>>> application". We've been doing this for 2-3 years now since we
>>>>> introduced Checkstyle and basically got nowhere. We should just bite
>> the
>>>>> bullet and get it over with; we could've solved this whole problem
>>>>> already.
>>>>>
>>>>> In conclusion, I'm +1 on finally locking down the codestyle and
>> applying
>>>>> it immediately, I'm -1 on any gradual application scheme because they
>>>>> _just don't work_.
>>>>>
>>>>> On 10/6/2020 2:15 PM, Aljoscha Krettek wrote:
>>>>>> Hi All,
>>>>>>
>>>>>> I know I know, but please keep reading because I recently learned
>>>>>> about some new developments in the area of coding-style automation.
>>>>>>
>>>>>> The tool I would propose we use is Spotless
>>>>>> (https://github.com/diffplug/spotless). This doesn't come with a
>>>>>> formatter but allows using other popular formatters such as
>>>>>> google-java-format. The nice thing about Spotless is that it serves as
>>>>>> a verifier for CI but can also apply the configured style
>>>>>> automatically. That is, for the programmer all she has to do is `mvn
>>>>>> spotless:apply` to fix any style violations.
>>>>>>
>>>>>> An interesting feature, which was (somewhat) recently added is
>>>>>> "ratchet"
>>>>>> (
>> https://github.com/diffplug/spotless/blob/main/plugin-maven/README.md#ratchet
>> ).
>>>>>> With this, you can set up Spotless to only apply it's rules to files
>>>>>> that were changed after a configured commit. This would allow a
>>>>>> gradual application of the new coding style instead of one big change.
>>>>>>
>>>>>> If we decide to use Spotless, we would of course also have to decide
>>>>>> on a coding style. For this I would propose google-java-format, which
>>>>>> the flink-statefun project uses. The main difference from our current
>>>>>> "style" is that this uses spaces instead of tabs for indentation. By
>>>>>> default it would be 2 spaces but it can be configured to use 4 spaces
>>>>>> which would make code look more or less like our current style. There
>>>>>> are no more configuration knobs, so using tabs is not an option.
>>>>>>
>>>>>> Finally, why should we do this? I think most engineers agree that
>>>>>> having a common enforced style is good to have so I only want to
>>>>>> highlight a few thoughts here about things we could improve:
>>>>>>
>>>>>>    - No more nits about coding style in reviews, this makes it easier
>>>>>> for both the reviewer and developer
>>>>>>
>>>>>>    - No manual fixing of Checkstyle errors because Spotless can do that
>>>>>> automatically
>>>>>>
>>>>>>    - Because Flink is such a big project little islands of coding style
>>>>>> have formed between people that commonly work on components. It can be
>>>>>> a nuisance when you work on a different component and then reviewers
>>>>>> don't like your typical coding style. And you first have to get used
>>>>>> to the slight differences in style when reading code.
>>>>>>
>>>>>> There are also downsides I see in this:
>>>>>>
>>>>>>    - We break the history, but both "git blame" and modern IntelliJ can
>>>>>> ignore whitespace when attributing changes. So for files that are
>>>>>> already "well" formatted not much would change.
>>>>>>
>>>>>>    - In the short-term it will be harder to apply changes both to
>> master
>>>>>> and one of the release-x branches because formatting will be
>>>>>> different. I think this is not too hard though because Spotless can
>>>>>> automatically apply the style.
>>>>>>
>>>>>> In summary, we would have some short-term pain with this but I think
>>>>>> it would be good in the long run. What are your thoughts?
>>>>>>
>>>>>> Best,
>>>>>> Aljoscha
>>>>>>
>>>>>
>>>> --
>>>>
>>>> Arvid Heise | Senior Java Developer
>>>>
>>>> <https://www.ververica.com/>
>>>>
>>>> Follow us @VervericaData
>>>>
>>>> --
>>>>
>>>> Join Flink Forward <https://flink-forward.org/> - The Apache Flink
>>>> Conference
>>>>
>>>> Stream Processing | Event Driven | Real Time
>>>>
>>>> --
>>>>
>>>> Ververica GmbH | Invalidenstrasse 115, 10115 Berlin, Germany
>>>>
>>>> --
>>>> Ververica GmbH
>>>> Registered at Amtsgericht Charlottenburg: HRB 158244 B
>>>> Managing Directors: Timothy Alexander Steinert, Yip Park Tung Jason, Ji
>>>> (Toni) Cheng
>>>>
>>>

[Kostas Kloudas-4]

Hi all,

+1 for enforcing "a" codestyle using "a" tool.

As the project grows both in terms of LOCs and contributors, this
becomes more and more important as it eliminates some potential points
of friction without any additional effort.

From the discussion, I am leaning towards having a single commit with
all the codestyle-related changes. This will avoid sprinkling
refactoring commits all over the place for the next year or more. But
if this is the price to pay for having consensus on a tool, then I am
ok with gradual implementation. I believe that the value added by
having an automated process of enforcing a codestyle exceeds the cost
of the nuisance of gradual refactoring.

As for the actual format, I like the google-java-format but again, if
the community agrees on a different one I would not oppose that (as
long as it does not use the same amount of indentation for method args
and method body :P).

Cheers,
Kostas

On Wed, Oct 7, 2020 at 10:26 AM Chesnay Schepler <[hidden email]> wrote:

>
> To me, ratchet seems to combine the worst aspects of both approaches.
> You get disruptive changes, but only in singular files,
> for something mundane as a typo fix or import change, which would be
> annoying to keep separate from the actual functional changes in a PR.
>
> On 10/7/2020 10:04 AM, Matthias Pohl wrote:
> > I find the ratchet feature you're suggesting interesting. But Arvid has a
> > point referring to the blog post about ignoring revisions in git blame [1].
> > Adding the configuration file for commits to ignore revs as proposed in the
> > blog post makes it even easier. One problem I see is that this is not
> > supported by Github (yet?) [2] as mentioned in [1].
> >
> > Considering all that I prefer applying the code style in one go. I have no
> > strong opinion on what codestyle is the best.
> >
> > PS: We used spotless in the project I previously worked on. It was
> > convenient to use.
> >
> > [1]
> > https://www.moxio.com/blog/43/ignoring-bulk-change-commits-with-git-blame
> > [2]
> > https://github.community/t/support-ignore-revs-file-in-githubs-blame-view/3256
> >
> > On Tue, Oct 6, 2020 at 6:00 PM Aljoscha Krettek <[hidden email]> wrote:
> >
> >> Maybe I wasn't very clear on how the ratchet works. The changes are
> >> gradual yes, but it doesn't leave you an option: if you touch a file you
> >> will it will have to conform to the coding style afterwards. It's not
> >> like the previous gradual process we had before where it would be based
> >> on people actively working towards a style.
> >>
> >> That being said, I also completely like the option of just doing one big
> >> change commit.
> >>
> >> Regarding actual coding styles: we're a bit limited by what tools exist.
> >> I like Spotless because it can be used to both check and apply a style.
> >> Then you need a formatter that works with Spotless and of those we only
> >> have the Eclipse Formatter, google-java-format, and Prettier. Prettier
> >> is a Javascript tool that I would like to avoid. Eclipse is doable but
> >> you need to fiddle with configuration files. I like google-java-format
> >> because of it's take-it-or-leave-it approach. You either use the style
> >> or you don't but it's very thorough. The downside is that it won't do
> >> tabs-only formatting.
> >>
> >> Best,
> >> Aljoscha
> >>
> >> On 06.10.20 17:43, Arvid Heise wrote:
> >>> After having written that I did a quick search, you can even use git
> >> blame
> >>> with one big massive change commit [1], which would further help the idea
> >>> of "just get over with it".
> >>>
> >>> With that option, I'd even change all whitespaces if the community thinks
> >>> that it's a better option (a separate discussion that I'll gladly skip).
> >>>
> >>> [1]
> >>>
> >> https://www.moxio.com/blog/43/ignoring-bulk-change-commits-with-git-blame
> >>> On Tue, Oct 6, 2020 at 5:38 PM Arvid Heise <[hidden email]> wrote:
> >>>
> >>>> I'm also +1 for automatically enforceable code style.
> >>>>
> >>>> I also would just go over it as Chesnay said. While it makes some
> >> changes
> >>>> a bit harder to track (inline git blame), it's easy to skip over in any
> >> git
> >>>> history and if it's only one massive commit, then it's also much easier
> >> to
> >>>> ignore than many gradual changes. Further, if we just do it once, git
> >> blame
> >>>> will quickly become more reliable again.
> >>>>
> >>>> Btw I completely don't care about the code style as long as it plays
> >> well
> >>>> with IntelliJ (it used to be different, but things change :p).
> >>>>
> >>>> On Tue, Oct 6, 2020 at 5:23 PM Chesnay Schepler <[hidden email]>
> >>>> wrote:
> >>>>
> >>>>> We shouldn't switch to spaces _now_; cutting this bit from your
> >> proposal
> >>>>> will massively simplify things and there's hardly any value in changing
> >>>>> it.
> >>>>>
> >>>>> Also I'm getting rather tired of this constant idea of "gradual
> >>>>> application". We've been doing this for 2-3 years now since we
> >>>>> introduced Checkstyle and basically got nowhere. We should just bite
> >> the
> >>>>> bullet and get it over with; we could've solved this whole problem
> >>>>> already.
> >>>>>
> >>>>> In conclusion, I'm +1 on finally locking down the codestyle and
> >> applying
> >>>>> it immediately, I'm -1 on any gradual application scheme because they
> >>>>> _just don't work_.
> >>>>>
> >>>>> On 10/6/2020 2:15 PM, Aljoscha Krettek wrote:
> >>>>>> Hi All,
> >>>>>>
> >>>>>> I know I know, but please keep reading because I recently learned
> >>>>>> about some new developments in the area of coding-style automation.
> >>>>>>
> >>>>>> The tool I would propose we use is Spotless
> >>>>>> (https://github.com/diffplug/spotless). This doesn't come with a
> >>>>>> formatter but allows using other popular formatters such as
> >>>>>> google-java-format. The nice thing about Spotless is that it serves as
> >>>>>> a verifier for CI but can also apply the configured style
> >>>>>> automatically. That is, for the programmer all she has to do is `mvn
> >>>>>> spotless:apply` to fix any style violations.
> >>>>>>
> >>>>>> An interesting feature, which was (somewhat) recently added is
> >>>>>> "ratchet"
> >>>>>> (
> >> https://github.com/diffplug/spotless/blob/main/plugin-maven/README.md#ratchet
> >> ).
> >>>>>> With this, you can set up Spotless to only apply it's rules to files
> >>>>>> that were changed after a configured commit. This would allow a
> >>>>>> gradual application of the new coding style instead of one big change.
> >>>>>>
> >>>>>> If we decide to use Spotless, we would of course also have to decide
> >>>>>> on a coding style. For this I would propose google-java-format, which
> >>>>>> the flink-statefun project uses. The main difference from our current
> >>>>>> "style" is that this uses spaces instead of tabs for indentation. By
> >>>>>> default it would be 2 spaces but it can be configured to use 4 spaces
> >>>>>> which would make code look more or less like our current style. There
> >>>>>> are no more configuration knobs, so using tabs is not an option.
> >>>>>>
> >>>>>> Finally, why should we do this? I think most engineers agree that
> >>>>>> having a common enforced style is good to have so I only want to
> >>>>>> highlight a few thoughts here about things we could improve:
> >>>>>>
> >>>>>>    - No more nits about coding style in reviews, this makes it easier
> >>>>>> for both the reviewer and developer
> >>>>>>
> >>>>>>    - No manual fixing of Checkstyle errors because Spotless can do that
> >>>>>> automatically
> >>>>>>
> >>>>>>    - Because Flink is such a big project little islands of coding style
> >>>>>> have formed between people that commonly work on components. It can be
> >>>>>> a nuisance when you work on a different component and then reviewers
> >>>>>> don't like your typical coding style. And you first have to get used
> >>>>>> to the slight differences in style when reading code.
> >>>>>>
> >>>>>> There are also downsides I see in this:
> >>>>>>
> >>>>>>    - We break the history, but both "git blame" and modern IntelliJ can
> >>>>>> ignore whitespace when attributing changes. So for files that are
> >>>>>> already "well" formatted not much would change.
> >>>>>>
> >>>>>>    - In the short-term it will be harder to apply changes both to
> >> master
> >>>>>> and one of the release-x branches because formatting will be
> >>>>>> different. I think this is not too hard though because Spotless can
> >>>>>> automatically apply the style.
> >>>>>>
> >>>>>> In summary, we would have some short-term pain with this but I think
> >>>>>> it would be good in the long run. What are your thoughts?
> >>>>>>
> >>>>>> Best,
> >>>>>> Aljoscha
> >>>>>>
> >>>>>
> >>>> --
> >>>>
> >>>> Arvid Heise | Senior Java Developer
> >>>>
> >>>> <https://www.ververica.com/>
> >>>>
> >>>> Follow us @VervericaData
> >>>>
> >>>> --
> >>>>
> >>>> Join Flink Forward <https://flink-forward.org/> - The Apache Flink
> >>>> Conference
> >>>>
> >>>> Stream Processing | Event Driven | Real Time
> >>>>
> >>>> --
> >>>>
> >>>> Ververica GmbH | Invalidenstrasse 115, 10115 Berlin, Germany
> >>>>
> >>>> --
> >>>> Ververica GmbH
> >>>> Registered at Amtsgericht Charlottenburg: HRB 158244 B
> >>>> Managing Directors: Timothy Alexander Steinert, Yip Park Tung Jason, Ji
> >>>> (Toni) Cheng
> >>>>
> >>>
>

[tison]

+1 on locking on one codestyle and I think that is what current checkstyle
rules serving.

For automatic applying part, we suggest developing by IDEA and with
Checkstyle Plugin on IDEA applying checkstyle suggestion is also automatic.

One short coming for spotless is that we can hardly adjust rules if the
project has its own issues to overcome. IIRC only several pre-defined rules
and a clumsy general regex substitution rule can be used.

FYI my team started with spotless but ended up with checkstyle with few
rules and Checkstyle Plugin for automation.

Codestyle, in another perspective, is part of cognition of developers
working in project, not something we just apply before pull request. No
matter how much automation introduced, most of developers will converge
working with the configured codestyle.

Best,
tison.


Kostas Kloudas <[hidden email]> 于2020年10月7日周三 下午6:37写道：

> Hi all,
>
> +1 for enforcing "a" codestyle using "a" tool.
>
> As the project grows both in terms of LOCs and contributors, this
> becomes more and more important as it eliminates some potential points
> of friction without any additional effort.
>
> From the discussion, I am leaning towards having a single commit with
> all the codestyle-related changes. This will avoid sprinkling
> refactoring commits all over the place for the next year or more. But
> if this is the price to pay for having consensus on a tool, then I am
> ok with gradual implementation. I believe that the value added by
> having an automated process of enforcing a codestyle exceeds the cost
> of the nuisance of gradual refactoring.
>
> As for the actual format, I like the google-java-format but again, if
> the community agrees on a different one I would not oppose that (as
> long as it does not use the same amount of indentation for method args
> and method body :P).
>
> Cheers,
> Kostas
>
> On Wed, Oct 7, 2020 at 10:26 AM Chesnay Schepler <[hidden email]>
> wrote:
> >
> > To me, ratchet seems to combine the worst aspects of both approaches.
> > You get disruptive changes, but only in singular files,
> > for something mundane as a typo fix or import change, which would be
> > annoying to keep separate from the actual functional changes in a PR.
> >
> > On 10/7/2020 10:04 AM, Matthias Pohl wrote:
> > > I find the ratchet feature you're suggesting interesting. But Arvid
> has a
> > > point referring to the blog post about ignoring revisions in git blame
> [1].
> > > Adding the configuration file for commits to ignore revs as proposed
> in the
> > > blog post makes it even easier. One problem I see is that this is not
> > > supported by Github (yet?) [2] as mentioned in [1].
> > >
> > > Considering all that I prefer applying the code style in one go. I
> have no
> > > strong opinion on what codestyle is the best.
> > >
> > > PS: We used spotless in the project I previously worked on. It was
> > > convenient to use.
> > >
> > > [1]
> > >
> https://www.moxio.com/blog/43/ignoring-bulk-change-commits-with-git-blame
> > > [2]
> > >
> https://github.community/t/support-ignore-revs-file-in-githubs-blame-view/3256
> > >
> > > On Tue, Oct 6, 2020 at 6:00 PM Aljoscha Krettek <[hidden email]>
> wrote:
> > >
> > >> Maybe I wasn't very clear on how the ratchet works. The changes are
> > >> gradual yes, but it doesn't leave you an option: if you touch a file
> you
> > >> will it will have to conform to the coding style afterwards. It's not
> > >> like the previous gradual process we had before where it would be
> based
> > >> on people actively working towards a style.
> > >>
> > >> That being said, I also completely like the option of just doing one
> big
> > >> change commit.
> > >>
> > >> Regarding actual coding styles: we're a bit limited by what tools
> exist.
> > >> I like Spotless because it can be used to both check and apply a
> style.
> > >> Then you need a formatter that works with Spotless and of those we
> only
> > >> have the Eclipse Formatter, google-java-format, and Prettier. Prettier
> > >> is a Javascript tool that I would like to avoid. Eclipse is doable but
> > >> you need to fiddle with configuration files. I like google-java-format
> > >> because of it's take-it-or-leave-it approach. You either use the style
> > >> or you don't but it's very thorough. The downside is that it won't do
> > >> tabs-only formatting.
> > >>
> > >> Best,
> > >> Aljoscha
> > >>
> > >> On 06.10.20 17:43, Arvid Heise wrote:
> > >>> After having written that I did a quick search, you can even use git
> > >> blame
> > >>> with one big massive change commit [1], which would further help the
> idea
> > >>> of "just get over with it".
> > >>>
> > >>> With that option, I'd even change all whitespaces if the community
> thinks
> > >>> that it's a better option (a separate discussion that I'll gladly
> skip).
> > >>>
> > >>> [1]
> > >>>
> > >>
> https://www.moxio.com/blog/43/ignoring-bulk-change-commits-with-git-blame
> > >>> On Tue, Oct 6, 2020 at 5:38 PM Arvid Heise <[hidden email]>
> wrote:
> > >>>
> > >>>> I'm also +1 for automatically enforceable code style.
> > >>>>
> > >>>> I also would just go over it as Chesnay said. While it makes some
> > >> changes
> > >>>> a bit harder to track (inline git blame), it's easy to skip over in
> any
> > >> git
> > >>>> history and if it's only one massive commit, then it's also much
> easier
> > >> to
> > >>>> ignore than many gradual changes. Further, if we just do it once,
> git
> > >> blame
> > >>>> will quickly become more reliable again.
> > >>>>
> > >>>> Btw I completely don't care about the code style as long as it plays
> > >> well
> > >>>> with IntelliJ (it used to be different, but things change :p).
> > >>>>
> > >>>> On Tue, Oct 6, 2020 at 5:23 PM Chesnay Schepler <[hidden email]
> >
> > >>>> wrote:
> > >>>>
> > >>>>> We shouldn't switch to spaces _now_; cutting this bit from your
> > >> proposal
> > >>>>> will massively simplify things and there's hardly any value in
> changing
> > >>>>> it.
> > >>>>>
> > >>>>> Also I'm getting rather tired of this constant idea of "gradual
> > >>>>> application". We've been doing this for 2-3 years now since we
> > >>>>> introduced Checkstyle and basically got nowhere. We should just
> bite
> > >> the
> > >>>>> bullet and get it over with; we could've solved this whole problem
> > >>>>> already.
> > >>>>>
> > >>>>> In conclusion, I'm +1 on finally locking down the codestyle and
> > >> applying
> > >>>>> it immediately, I'm -1 on any gradual application scheme because
> they
> > >>>>> _just don't work_.
> > >>>>>
> > >>>>> On 10/6/2020 2:15 PM, Aljoscha Krettek wrote:
> > >>>>>> Hi All,
> > >>>>>>
> > >>>>>> I know I know, but please keep reading because I recently learned
> > >>>>>> about some new developments in the area of coding-style
> automation.
> > >>>>>>
> > >>>>>> The tool I would propose we use is Spotless
> > >>>>>> (https://github.com/diffplug/spotless). This doesn't come with a
> > >>>>>> formatter but allows using other popular formatters such as
> > >>>>>> google-java-format. The nice thing about Spotless is that it
> serves as
> > >>>>>> a verifier for CI but can also apply the configured style
> > >>>>>> automatically. That is, for the programmer all she has to do is
> `mvn
> > >>>>>> spotless:apply` to fix any style violations.
> > >>>>>>
> > >>>>>> An interesting feature, which was (somewhat) recently added is
> > >>>>>> "ratchet"
> > >>>>>> (
> > >>
> https://github.com/diffplug/spotless/blob/main/plugin-maven/README.md#ratchet
> > >> ).
> > >>>>>> With this, you can set up Spotless to only apply it's rules to
> files
> > >>>>>> that were changed after a configured commit. This would allow a
> > >>>>>> gradual application of the new coding style instead of one big
> change.
> > >>>>>>
> > >>>>>> If we decide to use Spotless, we would of course also have to
> decide
> > >>>>>> on a coding style. For this I would propose google-java-format,
> which
> > >>>>>> the flink-statefun project uses. The main difference from our
> current
> > >>>>>> "style" is that this uses spaces instead of tabs for indentation.
> By
> > >>>>>> default it would be 2 spaces but it can be configured to use 4
> spaces
> > >>>>>> which would make code look more or less like our current style.
> There
> > >>>>>> are no more configuration knobs, so using tabs is not an option.
> > >>>>>>
> > >>>>>> Finally, why should we do this? I think most engineers agree that
> > >>>>>> having a common enforced style is good to have so I only want to
> > >>>>>> highlight a few thoughts here about things we could improve:
> > >>>>>>
> > >>>>>>    - No more nits about coding style in reviews, this makes it
> easier
> > >>>>>> for both the reviewer and developer
> > >>>>>>
> > >>>>>>    - No manual fixing of Checkstyle errors because Spotless can
> do that
> > >>>>>> automatically
> > >>>>>>
> > >>>>>>    - Because Flink is such a big project little islands of coding
> style
> > >>>>>> have formed between people that commonly work on components. It
> can be
> > >>>>>> a nuisance when you work on a different component and then
> reviewers
> > >>>>>> don't like your typical coding style. And you first have to get
> used
> > >>>>>> to the slight differences in style when reading code.
> > >>>>>>
> > >>>>>> There are also downsides I see in this:
> > >>>>>>
> > >>>>>>    - We break the history, but both "git blame" and modern
> IntelliJ can
> > >>>>>> ignore whitespace when attributing changes. So for files that are
> > >>>>>> already "well" formatted not much would change.
> > >>>>>>
> > >>>>>>    - In the short-term it will be harder to apply changes both to
> > >> master
> > >>>>>> and one of the release-x branches because formatting will be
> > >>>>>> different. I think this is not too hard though because Spotless
> can
> > >>>>>> automatically apply the style.
> > >>>>>>
> > >>>>>> In summary, we would have some short-term pain with this but I
> think
> > >>>>>> it would be good in the long run. What are your thoughts?
> > >>>>>>
> > >>>>>> Best,
> > >>>>>> Aljoscha
> > >>>>>>
> > >>>>>
> > >>>> --
> > >>>>
> > >>>> Arvid Heise | Senior Java Developer
> > >>>>
> > >>>> <https://www.ververica.com/>
> > >>>>
> > >>>> Follow us @VervericaData
> > >>>>
> > >>>> --
> > >>>>
> > >>>> Join Flink Forward <https://flink-forward.org/> - The Apache Flink
> > >>>> Conference
> > >>>>
> > >>>> Stream Processing | Event Driven | Real Time
> > >>>>
> > >>>> --
> > >>>>
> > >>>> Ververica GmbH | Invalidenstrasse 115, 10115 Berlin, Germany
> > >>>>
> > >>>> --
> > >>>> Ververica GmbH
> > >>>> Registered at Amtsgericht Charlottenburg: HRB 158244 B
> > >>>> Managing Directors: Timothy Alexander Steinert, Yip Park Tung
> Jason, Ji
> > >>>> (Toni) Cheng
> > >>>>
> > >>>
> >
>

[Aljoscha Krettek-2]

I don't like checkstyle because it cannot be easily applied from the
commandline. I'm happy to learn otherwise, though. And I'd also be very
happy about alternative suggestions that can do that.

Right now, I think Spotless is the most straightforward tool for that.
Also I don't care so much about what style we choose in the end. (If we
choose one). My main motivation is that we have a common, strict style
that can easily applied via tooling so that we no longer need to comment
on coding style in PRs.

Aljoscha

On 09.10.20 11:11, tison wrote:

> +1 on locking on one codestyle and I think that is what current checkstyle
> rules serving.
>
> For automatic applying part, we suggest developing by IDEA and with
> Checkstyle Plugin on IDEA applying checkstyle suggestion is also automatic.
>
> One short coming for spotless is that we can hardly adjust rules if the
> project has its own issues to overcome. IIRC only several pre-defined rules
> and a clumsy general regex substitution rule can be used.
>
> FYI my team started with spotless but ended up with checkstyle with few
> rules and Checkstyle Plugin for automation.
>
> Codestyle, in another perspective, is part of cognition of developers
> working in project, not something we just apply before pull request. No
> matter how much automation introduced, most of developers will converge
> working with the configured codestyle.
>
> Best,
> tison.
>
>
> Kostas Kloudas <[hidden email]> 于2020年10月7日周三 下午6:37写道：
>
>> Hi all,
>>
>> +1 for enforcing "a" codestyle using "a" tool.
>>
>> As the project grows both in terms of LOCs and contributors, this
>> becomes more and more important as it eliminates some potential points
>> of friction without any additional effort.
>>
>>  From the discussion, I am leaning towards having a single commit with
>> all the codestyle-related changes. This will avoid sprinkling
>> refactoring commits all over the place for the next year or more. But
>> if this is the price to pay for having consensus on a tool, then I am
>> ok with gradual implementation. I believe that the value added by
>> having an automated process of enforcing a codestyle exceeds the cost
>> of the nuisance of gradual refactoring.
>>
>> As for the actual format, I like the google-java-format but again, if
>> the community agrees on a different one I would not oppose that (as
>> long as it does not use the same amount of indentation for method args
>> and method body :P).
>>
>> Cheers,
>> Kostas
>>
>> On Wed, Oct 7, 2020 at 10:26 AM Chesnay Schepler <[hidden email]>
>> wrote:
>>>
>>> To me, ratchet seems to combine the worst aspects of both approaches.
>>> You get disruptive changes, but only in singular files,
>>> for something mundane as a typo fix or import change, which would be
>>> annoying to keep separate from the actual functional changes in a PR.
>>>
>>> On 10/7/2020 10:04 AM, Matthias Pohl wrote:
>>>> I find the ratchet feature you're suggesting interesting. But Arvid
>> has a
>>>> point referring to the blog post about ignoring revisions in git blame
>> [1].
>>>> Adding the configuration file for commits to ignore revs as proposed
>> in the
>>>> blog post makes it even easier. One problem I see is that this is not
>>>> supported by Github (yet?) [2] as mentioned in [1].
>>>>
>>>> Considering all that I prefer applying the code style in one go. I
>> have no
>>>> strong opinion on what codestyle is the best.
>>>>
>>>> PS: We used spotless in the project I previously worked on. It was
>>>> convenient to use.
>>>>
>>>> [1]
>>>>
>> https://www.moxio.com/blog/43/ignoring-bulk-change-commits-with-git-blame
>>>> [2]
>>>>
>> https://github.community/t/support-ignore-revs-file-in-githubs-blame-view/3256
>>>>
>>>> On Tue, Oct 6, 2020 at 6:00 PM Aljoscha Krettek <[hidden email]>
>> wrote:
>>>>
>>>>> Maybe I wasn't very clear on how the ratchet works. The changes are
>>>>> gradual yes, but it doesn't leave you an option: if you touch a file
>> you
>>>>> will it will have to conform to the coding style afterwards. It's not
>>>>> like the previous gradual process we had before where it would be
>> based
>>>>> on people actively working towards a style.
>>>>>
>>>>> That being said, I also completely like the option of just doing one
>> big
>>>>> change commit.
>>>>>
>>>>> Regarding actual coding styles: we're a bit limited by what tools
>> exist.
>>>>> I like Spotless because it can be used to both check and apply a
>> style.
>>>>> Then you need a formatter that works with Spotless and of those we
>> only
>>>>> have the Eclipse Formatter, google-java-format, and Prettier. Prettier
>>>>> is a Javascript tool that I would like to avoid. Eclipse is doable but
>>>>> you need to fiddle with configuration files. I like google-java-format
>>>>> because of it's take-it-or-leave-it approach. You either use the style
>>>>> or you don't but it's very thorough. The downside is that it won't do
>>>>> tabs-only formatting.
>>>>>
>>>>> Best,
>>>>> Aljoscha
>>>>>
>>>>> On 06.10.20 17:43, Arvid Heise wrote:
>>>>>> After having written that I did a quick search, you can even use git
>>>>> blame
>>>>>> with one big massive change commit [1], which would further help the
>> idea
>>>>>> of "just get over with it".
>>>>>>
>>>>>> With that option, I'd even change all whitespaces if the community
>> thinks
>>>>>> that it's a better option (a separate discussion that I'll gladly
>> skip).
>>>>>>
>>>>>> [1]
>>>>>>
>>>>>
>> https://www.moxio.com/blog/43/ignoring-bulk-change-commits-with-git-blame
>>>>>> On Tue, Oct 6, 2020 at 5:38 PM Arvid Heise <[hidden email]>
>> wrote:
>>>>>>
>>>>>>> I'm also +1 for automatically enforceable code style.
>>>>>>>
>>>>>>> I also would just go over it as Chesnay said. While it makes some
>>>>> changes
>>>>>>> a bit harder to track (inline git blame), it's easy to skip over in
>> any
>>>>> git
>>>>>>> history and if it's only one massive commit, then it's also much
>> easier
>>>>> to
>>>>>>> ignore than many gradual changes. Further, if we just do it once,
>> git
>>>>> blame
>>>>>>> will quickly become more reliable again.
>>>>>>>
>>>>>>> Btw I completely don't care about the code style as long as it plays
>>>>> well
>>>>>>> with IntelliJ (it used to be different, but things change :p).
>>>>>>>
>>>>>>> On Tue, Oct 6, 2020 at 5:23 PM Chesnay Schepler <[hidden email]
>>>
>>>>>>> wrote:
>>>>>>>
>>>>>>>> We shouldn't switch to spaces _now_; cutting this bit from your
>>>>> proposal
>>>>>>>> will massively simplify things and there's hardly any value in
>> changing
>>>>>>>> it.
>>>>>>>>
>>>>>>>> Also I'm getting rather tired of this constant idea of "gradual
>>>>>>>> application". We've been doing this for 2-3 years now since we
>>>>>>>> introduced Checkstyle and basically got nowhere. We should just
>> bite
>>>>> the
>>>>>>>> bullet and get it over with; we could've solved this whole problem
>>>>>>>> already.
>>>>>>>>
>>>>>>>> In conclusion, I'm +1 on finally locking down the codestyle and
>>>>> applying
>>>>>>>> it immediately, I'm -1 on any gradual application scheme because
>> they
>>>>>>>> _just don't work_.
>>>>>>>>
>>>>>>>> On 10/6/2020 2:15 PM, Aljoscha Krettek wrote:
>>>>>>>>> Hi All,
>>>>>>>>>
>>>>>>>>> I know I know, but please keep reading because I recently learned
>>>>>>>>> about some new developments in the area of coding-style
>> automation.
>>>>>>>>>
>>>>>>>>> The tool I would propose we use is Spotless
>>>>>>>>> (https://github.com/diffplug/spotless). This doesn't come with a
>>>>>>>>> formatter but allows using other popular formatters such as
>>>>>>>>> google-java-format. The nice thing about Spotless is that it
>> serves as
>>>>>>>>> a verifier for CI but can also apply the configured style
>>>>>>>>> automatically. That is, for the programmer all she has to do is
>> `mvn
>>>>>>>>> spotless:apply` to fix any style violations.
>>>>>>>>>
>>>>>>>>> An interesting feature, which was (somewhat) recently added is
>>>>>>>>> "ratchet"
>>>>>>>>> (
>>>>>
>> https://github.com/diffplug/spotless/blob/main/plugin-maven/README.md#ratchet
>>>>> ).
>>>>>>>>> With this, you can set up Spotless to only apply it's rules to
>> files
>>>>>>>>> that were changed after a configured commit. This would allow a
>>>>>>>>> gradual application of the new coding style instead of one big
>> change.
>>>>>>>>>
>>>>>>>>> If we decide to use Spotless, we would of course also have to
>> decide
>>>>>>>>> on a coding style. For this I would propose google-java-format,
>> which
>>>>>>>>> the flink-statefun project uses. The main difference from our
>> current
>>>>>>>>> "style" is that this uses spaces instead of tabs for indentation.
>> By
>>>>>>>>> default it would be 2 spaces but it can be configured to use 4
>> spaces
>>>>>>>>> which would make code look more or less like our current style.
>> There
>>>>>>>>> are no more configuration knobs, so using tabs is not an option.
>>>>>>>>>
>>>>>>>>> Finally, why should we do this? I think most engineers agree that
>>>>>>>>> having a common enforced style is good to have so I only want to
>>>>>>>>> highlight a few thoughts here about things we could improve:
>>>>>>>>>
>>>>>>>>>     - No more nits about coding style in reviews, this makes it
>> easier
>>>>>>>>> for both the reviewer and developer
>>>>>>>>>
>>>>>>>>>     - No manual fixing of Checkstyle errors because Spotless can
>> do that
>>>>>>>>> automatically
>>>>>>>>>
>>>>>>>>>     - Because Flink is such a big project little islands of coding
>> style
>>>>>>>>> have formed between people that commonly work on components. It
>> can be
>>>>>>>>> a nuisance when you work on a different component and then
>> reviewers
>>>>>>>>> don't like your typical coding style. And you first have to get
>> used
>>>>>>>>> to the slight differences in style when reading code.
>>>>>>>>>
>>>>>>>>> There are also downsides I see in this:
>>>>>>>>>
>>>>>>>>>     - We break the history, but both "git blame" and modern
>> IntelliJ can
>>>>>>>>> ignore whitespace when attributing changes. So for files that are
>>>>>>>>> already "well" formatted not much would change.
>>>>>>>>>
>>>>>>>>>     - In the short-term it will be harder to apply changes both to
>>>>> master
>>>>>>>>> and one of the release-x branches because formatting will be
>>>>>>>>> different. I think this is not too hard though because Spotless
>> can
>>>>>>>>> automatically apply the style.
>>>>>>>>>
>>>>>>>>> In summary, we would have some short-term pain with this but I
>> think
>>>>>>>>> it would be good in the long run. What are your thoughts?
>>>>>>>>>
>>>>>>>>> Best,
>>>>>>>>> Aljoscha
>>>>>>>>>
>>>>>>>>
>>>>>>> --
>>>>>>>
>>>>>>> Arvid Heise | Senior Java Developer
>>>>>>>
>>>>>>> <https://www.ververica.com/>
>>>>>>>
>>>>>>> Follow us @VervericaData
>>>>>>>
>>>>>>> --
>>>>>>>
>>>>>>> Join Flink Forward <https://flink-forward.org/> - The Apache Flink
>>>>>>> Conference
>>>>>>>
>>>>>>> Stream Processing | Event Driven | Real Time
>>>>>>>
>>>>>>> --
>>>>>>>
>>>>>>> Ververica GmbH | Invalidenstrasse 115, 10115 Berlin, Germany
>>>>>>>
>>>>>>> --
>>>>>>> Ververica GmbH
>>>>>>> Registered at Amtsgericht Charlottenburg: HRB 158244 B
>>>>>>> Managing Directors: Timothy Alexander Steinert, Yip Park Tung
>> Jason, Ji
>>>>>>> (Toni) Cheng
>>>>>>>
>>>>>>
>>>
>>
>

[Aljoscha Krettek-2]

Let's try and conclude this discussion! I've prepared a PoC that uses
Spotless with google-java-format to do the formatting:
https://github.com/aljoscha/flink/commits/flink-xxx-spotless

To summarize from earlier discussion, the main benefits are:
  - no more worrying about code style, both as reviewer and implementer
  - everyone can configure their IDE to auto-format, there will never be
unrelated formatting changes

Also, this uses git's blame.ignoreRevsFile to add a file that tells git
blame/IntelliJ to ignore the refactoring commit. However, you need to
manually configure your git for that using:

$ git config blame.ignoreRevsFile .git-blame-ignore-revs

You can check out the PoC, look at the code in an IDE, play around with
blame/annotations.

By the way, the coding style is not configurable, it’s the Google Java
Style, wich uses spaces for indentation. In an IDE or on github the code
looks barely different from the previous formatting at a first glance.

For IDE setup, I will add a guide in the README but it boils down to:

  - install the google-java-format plugin, enable it
  - install the Save Actions plugin, enable "Optimize Imports" and
"Reformat File"

With this, every time you save, formatting will be applied
automatically. You will never see formatting complaints from CI (except
for cases where you break semantical checkstyle rules, such as using
certain imports that we don't allow.).

What do you think?

Best,
Aljoscha

On 19.10.20 12:36, Aljoscha Krettek wrote:

> I don't like checkstyle because it cannot be easily applied from the
> commandline. I'm happy to learn otherwise, though. And I'd also be very
> happy about alternative suggestions that can do that.
>
> Right now, I think Spotless is the most straightforward tool for that.
> Also I don't care so much about what style we choose in the end. (If we
> choose one). My main motivation is that we have a common, strict style
> that can easily applied via tooling so that we no longer need to comment
> on coding style in PRs.
>
> Aljoscha
>
> On 09.10.20 11:11, tison wrote:
>> +1 on locking on one codestyle and I think that is what current
>> checkstyle
>> rules serving.
>>
>> For automatic applying part, we suggest developing by IDEA and with
>> Checkstyle Plugin on IDEA applying checkstyle suggestion is also
>> automatic.
>>
>> One short coming for spotless is that we can hardly adjust rules if the
>> project has its own issues to overcome. IIRC only several pre-defined
>> rules
>> and a clumsy general regex substitution rule can be used.
>>
>> FYI my team started with spotless but ended up with checkstyle with few
>> rules and Checkstyle Plugin for automation.
>>
>> Codestyle, in another perspective, is part of cognition of developers
>> working in project, not something we just apply before pull request. No
>> matter how much automation introduced, most of developers will converge
>> working with the configured codestyle.
>>
>> Best,
>> tison.
>>
>>
>> Kostas Kloudas <[hidden email]> 于2020年10月7日周三 下午6:37写道：
>>
>>> Hi all,
>>>
>>> +1 for enforcing "a" codestyle using "a" tool.
>>>
>>> As the project grows both in terms of LOCs and contributors, this
>>> becomes more and more important as it eliminates some potential points
>>> of friction without any additional effort.
>>>
>>>  From the discussion, I am leaning towards having a single commit with
>>> all the codestyle-related changes. This will avoid sprinkling
>>> refactoring commits all over the place for the next year or more. But
>>> if this is the price to pay for having consensus on a tool, then I am
>>> ok with gradual implementation. I believe that the value added by
>>> having an automated process of enforcing a codestyle exceeds the cost
>>> of the nuisance of gradual refactoring.
>>>
>>> As for the actual format, I like the google-java-format but again, if
>>> the community agrees on a different one I would not oppose that (as
>>> long as it does not use the same amount of indentation for method args
>>> and method body :P).
>>>
>>> Cheers,
>>> Kostas
>>>
>>> On Wed, Oct 7, 2020 at 10:26 AM Chesnay Schepler <[hidden email]>
>>> wrote:
>>>>
>>>> To me, ratchet seems to combine the worst aspects of both approaches.
>>>> You get disruptive changes, but only in singular files,
>>>> for something mundane as a typo fix or import change, which would be
>>>> annoying to keep separate from the actual functional changes in a PR.
>>>>
>>>> On 10/7/2020 10:04 AM, Matthias Pohl wrote:
>>>>> I find the ratchet feature you're suggesting interesting. But Arvid
>>> has a
>>>>> point referring to the blog post about ignoring revisions in git blame
>>> [1].
>>>>> Adding the configuration file for commits to ignore revs as proposed
>>> in the
>>>>> blog post makes it even easier. One problem I see is that this is not
>>>>> supported by Github (yet?) [2] as mentioned in [1].
>>>>>
>>>>> Considering all that I prefer applying the code style in one go. I
>>> have no
>>>>> strong opinion on what codestyle is the best.
>>>>>
>>>>> PS: We used spotless in the project I previously worked on. It was
>>>>> convenient to use.
>>>>>
>>>>> [1]
>>>>>
>>> https://www.moxio.com/blog/43/ignoring-bulk-change-commits-with-git-blame 
>>>
>>>>> [2]
>>>>>
>>> https://github.community/t/support-ignore-revs-file-in-githubs-blame-view/3256 
>>>
>>>>>
>>>>> On Tue, Oct 6, 2020 at 6:00 PM Aljoscha Krettek <[hidden email]>
>>> wrote:
>>>>>
>>>>>> Maybe I wasn't very clear on how the ratchet works. The changes are
>>>>>> gradual yes, but it doesn't leave you an option: if you touch a file
>>> you
>>>>>> will it will have to conform to the coding style afterwards. It's not
>>>>>> like the previous gradual process we had before where it would be
>>> based
>>>>>> on people actively working towards a style.
>>>>>>
>>>>>> That being said, I also completely like the option of just doing one
>>> big
>>>>>> change commit.
>>>>>>
>>>>>> Regarding actual coding styles: we're a bit limited by what tools
>>> exist.
>>>>>> I like Spotless because it can be used to both check and apply a
>>> style.
>>>>>> Then you need a formatter that works with Spotless and of those we
>>> only
>>>>>> have the Eclipse Formatter, google-java-format, and Prettier.
>>>>>> Prettier
>>>>>> is a Javascript tool that I would like to avoid. Eclipse is doable
>>>>>> but
>>>>>> you need to fiddle with configuration files. I like
>>>>>> google-java-format
>>>>>> because of it's take-it-or-leave-it approach. You either use the
>>>>>> style
>>>>>> or you don't but it's very thorough. The downside is that it won't do
>>>>>> tabs-only formatting.
>>>>>>
>>>>>> Best,
>>>>>> Aljoscha
>>>>>>
>>>>>> On 06.10.20 17:43, Arvid Heise wrote:
>>>>>>> After having written that I did a quick search, you can even use git
>>>>>> blame
>>>>>>> with one big massive change commit [1], which would further help the
>>> idea
>>>>>>> of "just get over with it".
>>>>>>>
>>>>>>> With that option, I'd even change all whitespaces if the community
>>> thinks
>>>>>>> that it's a better option (a separate discussion that I'll gladly
>>> skip).
>>>>>>>
>>>>>>> [1]
>>>>>>>
>>>>>>
>>> https://www.moxio.com/blog/43/ignoring-bulk-change-commits-with-git-blame 
>>>
>>>>>>> On Tue, Oct 6, 2020 at 5:38 PM Arvid Heise <[hidden email]>
>>> wrote:
>>>>>>>
>>>>>>>> I'm also +1 for automatically enforceable code style.
>>>>>>>>
>>>>>>>> I also would just go over it as Chesnay said. While it makes some
>>>>>> changes
>>>>>>>> a bit harder to track (inline git blame), it's easy to skip over in
>>> any
>>>>>> git
>>>>>>>> history and if it's only one massive commit, then it's also much
>>> easier
>>>>>> to
>>>>>>>> ignore than many gradual changes. Further, if we just do it once,
>>> git
>>>>>> blame
>>>>>>>> will quickly become more reliable again.
>>>>>>>>
>>>>>>>> Btw I completely don't care about the code style as long as it
>>>>>>>> plays
>>>>>> well
>>>>>>>> with IntelliJ (it used to be different, but things change :p).
>>>>>>>>
>>>>>>>> On Tue, Oct 6, 2020 at 5:23 PM Chesnay Schepler <[hidden email]
>>>>
>>>>>>>> wrote:
>>>>>>>>
>>>>>>>>> We shouldn't switch to spaces _now_; cutting this bit from your
>>>>>> proposal
>>>>>>>>> will massively simplify things and there's hardly any value in
>>> changing
>>>>>>>>> it.
>>>>>>>>>
>>>>>>>>> Also I'm getting rather tired of this constant idea of "gradual
>>>>>>>>> application". We've been doing this for 2-3 years now since we
>>>>>>>>> introduced Checkstyle and basically got nowhere. We should just
>>> bite
>>>>>> the
>>>>>>>>> bullet and get it over with; we could've solved this whole problem
>>>>>>>>> already.
>>>>>>>>>
>>>>>>>>> In conclusion, I'm +1 on finally locking down the codestyle and
>>>>>> applying
>>>>>>>>> it immediately, I'm -1 on any gradual application scheme because
>>> they
>>>>>>>>> _just don't work_.
>>>>>>>>>
>>>>>>>>> On 10/6/2020 2:15 PM, Aljoscha Krettek wrote:
>>>>>>>>>> Hi All,
>>>>>>>>>>
>>>>>>>>>> I know I know, but please keep reading because I recently learned
>>>>>>>>>> about some new developments in the area of coding-style
>>> automation.
>>>>>>>>>>
>>>>>>>>>> The tool I would propose we use is Spotless
>>>>>>>>>> (https://github.com/diffplug/spotless). This doesn't come with a
>>>>>>>>>> formatter but allows using other popular formatters such as
>>>>>>>>>> google-java-format. The nice thing about Spotless is that it
>>> serves as
>>>>>>>>>> a verifier for CI but can also apply the configured style
>>>>>>>>>> automatically. That is, for the programmer all she has to do is
>>> `mvn
>>>>>>>>>> spotless:apply` to fix any style violations.
>>>>>>>>>>
>>>>>>>>>> An interesting feature, which was (somewhat) recently added is
>>>>>>>>>> "ratchet"
>>>>>>>>>> (
>>>>>>
>>> https://github.com/diffplug/spotless/blob/main/plugin-maven/README.md#ratchet 
>>>
>>>>>> ).
>>>>>>>>>> With this, you can set up Spotless to only apply it's rules to
>>> files
>>>>>>>>>> that were changed after a configured commit. This would allow a
>>>>>>>>>> gradual application of the new coding style instead of one big
>>> change.
>>>>>>>>>>
>>>>>>>>>> If we decide to use Spotless, we would of course also have to
>>> decide
>>>>>>>>>> on a coding style. For this I would propose google-java-format,
>>> which
>>>>>>>>>> the flink-statefun project uses. The main difference from our
>>> current
>>>>>>>>>> "style" is that this uses spaces instead of tabs for indentation.
>>> By
>>>>>>>>>> default it would be 2 spaces but it can be configured to use 4
>>> spaces
>>>>>>>>>> which would make code look more or less like our current style.
>>> There
>>>>>>>>>> are no more configuration knobs, so using tabs is not an option.
>>>>>>>>>>
>>>>>>>>>> Finally, why should we do this? I think most engineers agree that
>>>>>>>>>> having a common enforced style is good to have so I only want to
>>>>>>>>>> highlight a few thoughts here about things we could improve:
>>>>>>>>>>
>>>>>>>>>>     - No more nits about coding style in reviews, this makes it
>>> easier
>>>>>>>>>> for both the reviewer and developer
>>>>>>>>>>
>>>>>>>>>>     - No manual fixing of Checkstyle errors because Spotless can
>>> do that
>>>>>>>>>> automatically
>>>>>>>>>>
>>>>>>>>>>     - Because Flink is such a big project little islands of
>>>>>>>>>> coding
>>> style
>>>>>>>>>> have formed between people that commonly work on components. It
>>> can be
>>>>>>>>>> a nuisance when you work on a different component and then
>>> reviewers
>>>>>>>>>> don't like your typical coding style. And you first have to get
>>> used
>>>>>>>>>> to the slight differences in style when reading code.
>>>>>>>>>>
>>>>>>>>>> There are also downsides I see in this:
>>>>>>>>>>
>>>>>>>>>>     - We break the history, but both "git blame" and modern
>>> IntelliJ can
>>>>>>>>>> ignore whitespace when attributing changes. So for files that are
>>>>>>>>>> already "well" formatted not much would change.
>>>>>>>>>>
>>>>>>>>>>     - In the short-term it will be harder to apply changes
>>>>>>>>>> both to
>>>>>> master
>>>>>>>>>> and one of the release-x branches because formatting will be
>>>>>>>>>> different. I think this is not too hard though because Spotless
>>> can
>>>>>>>>>> automatically apply the style.
>>>>>>>>>>
>>>>>>>>>> In summary, we would have some short-term pain with this but I
>>> think
>>>>>>>>>> it would be good in the long run. What are your thoughts?
>>>>>>>>>>
>>>>>>>>>> Best,
>>>>>>>>>> Aljoscha
>>>>>>>>>>
>>>>>>>>>
>>>>>>>> --
>>>>>>>>
>>>>>>>> Arvid Heise | Senior Java Developer
>>>>>>>>
>>>>>>>> <https://www.ververica.com/>
>>>>>>>>
>>>>>>>> Follow us @VervericaData
>>>>>>>>
>>>>>>>> --
>>>>>>>>
>>>>>>>> Join Flink Forward <https://flink-forward.org/> - The Apache Flink
>>>>>>>> Conference
>>>>>>>>
>>>>>>>> Stream Processing | Event Driven | Real Time
>>>>>>>>
>>>>>>>> --
>>>>>>>>
>>>>>>>> Ververica GmbH | Invalidenstrasse 115, 10115 Berlin, Germany
>>>>>>>>
>>>>>>>> --
>>>>>>>> Ververica GmbH
>>>>>>>> Registered at Amtsgericht Charlottenburg: HRB 158244 B
>>>>>>>> Managing Directors: Timothy Alexander Steinert, Yip Park Tung
>>> Jason, Ji
>>>>>>>> (Toni) Cheng
>>>>>>>>
>>>>>>>
>>>>
>>>
>>
>

[Chesnay Schepler-3]

+1 to set this up ASAP. Now's a good time to (finally) finalize this
effort with a new release cycle having started and christmas/vacations
being around the corner.

On 12/16/2020 7:20 PM, Aljoscha Krettek wrote:

> Let's try and conclude this discussion! I've prepared a PoC that uses
> Spotless with google-java-format to do the formatting:
> https://github.com/aljoscha/flink/commits/flink-xxx-spotless
>
> To summarize from earlier discussion, the main benefits are:
>  - no more worrying about code style, both as reviewer and implementer
>  - everyone can configure their IDE to auto-format, there will never
> be unrelated formatting changes
>
> Also, this uses git's blame.ignoreRevsFile to add a file that tells
> git blame/IntelliJ to ignore the refactoring commit. However, you need
> to manually configure your git for that using:
>
> $ git config blame.ignoreRevsFile .git-blame-ignore-revs
>
> You can check out the PoC, look at the code in an IDE, play around
> with blame/annotations.
>
> By the way, the coding style is not configurable, it’s the Google Java
> Style, wich uses spaces for indentation. In an IDE or on github the
> code looks barely different from the previous formatting at a first
> glance.
>
> For IDE setup, I will add a guide in the README but it boils down to:
>
>  - install the google-java-format plugin, enable it
>  - install the Save Actions plugin, enable "Optimize Imports" and
> "Reformat File"
>
> With this, every time you save, formatting will be applied
> automatically. You will never see formatting complaints from CI
> (except for cases where you break semantical checkstyle rules, such as
> using certain imports that we don't allow.).
>
> What do you think?
>
> Best,
> Aljoscha
>
> On 19.10.20 12:36, Aljoscha Krettek wrote:
>> I don't like checkstyle because it cannot be easily applied from the
>> commandline. I'm happy to learn otherwise, though. And I'd also be
>> very happy about alternative suggestions that can do that.
>>
>> Right now, I think Spotless is the most straightforward tool for
>> that. Also I don't care so much about what style we choose in the
>> end. (If we choose one). My main motivation is that we have a common,
>> strict style that can easily applied via tooling so that we no longer
>> need to comment on coding style in PRs.
>>
>> Aljoscha
>>
>> On 09.10.20 11:11, tison wrote:
>>> +1 on locking on one codestyle and I think that is what current
>>> checkstyle
>>> rules serving.
>>>
>>> For automatic applying part, we suggest developing by IDEA and with
>>> Checkstyle Plugin on IDEA applying checkstyle suggestion is also
>>> automatic.
>>>
>>> One short coming for spotless is that we can hardly adjust rules if the
>>> project has its own issues to overcome. IIRC only several
>>> pre-defined rules
>>> and a clumsy general regex substitution rule can be used.
>>>
>>> FYI my team started with spotless but ended up with checkstyle with few
>>> rules and Checkstyle Plugin for automation.
>>>
>>> Codestyle, in another perspective, is part of cognition of developers
>>> working in project, not something we just apply before pull request. No
>>> matter how much automation introduced, most of developers will converge
>>> working with the configured codestyle.
>>>
>>> Best,
>>> tison.
>>>
>>>
>>> Kostas Kloudas <[hidden email]> 于2020年10月7日周三 下午6:37写道：
>>>
>>>> Hi all,
>>>>
>>>> +1 for enforcing "a" codestyle using "a" tool.
>>>>
>>>> As the project grows both in terms of LOCs and contributors, this
>>>> becomes more and more important as it eliminates some potential points
>>>> of friction without any additional effort.
>>>>
>>>>  From the discussion, I am leaning towards having a single commit with
>>>> all the codestyle-related changes. This will avoid sprinkling
>>>> refactoring commits all over the place for the next year or more. But
>>>> if this is the price to pay for having consensus on a tool, then I am
>>>> ok with gradual implementation. I believe that the value added by
>>>> having an automated process of enforcing a codestyle exceeds the cost
>>>> of the nuisance of gradual refactoring.
>>>>
>>>> As for the actual format, I like the google-java-format but again, if
>>>> the community agrees on a different one I would not oppose that (as
>>>> long as it does not use the same amount of indentation for method args
>>>> and method body :P).
>>>>
>>>> Cheers,
>>>> Kostas
>>>>
>>>> On Wed, Oct 7, 2020 at 10:26 AM Chesnay Schepler <[hidden email]>
>>>> wrote:
>>>>>
>>>>> To me, ratchet seems to combine the worst aspects of both approaches.
>>>>> You get disruptive changes, but only in singular files,
>>>>> for something mundane as a typo fix or import change, which would be
>>>>> annoying to keep separate from the actual functional changes in a PR.
>>>>>
>>>>> On 10/7/2020 10:04 AM, Matthias Pohl wrote:
>>>>>> I find the ratchet feature you're suggesting interesting. But Arvid
>>>> has a
>>>>>> point referring to the blog post about ignoring revisions in git
>>>>>> blame
>>>> [1].
>>>>>> Adding the configuration file for commits to ignore revs as proposed
>>>> in the
>>>>>> blog post makes it even easier. One problem I see is that this is
>>>>>> not
>>>>>> supported by Github (yet?) [2] as mentioned in [1].
>>>>>>
>>>>>> Considering all that I prefer applying the code style in one go. I
>>>> have no
>>>>>> strong opinion on what codestyle is the best.
>>>>>>
>>>>>> PS: We used spotless in the project I previously worked on. It was
>>>>>> convenient to use.
>>>>>>
>>>>>> [1]
>>>>>>
>>>> https://www.moxio.com/blog/43/ignoring-bulk-change-commits-with-git-blame 
>>>>
>>>>>> [2]
>>>>>>
>>>> https://github.community/t/support-ignore-revs-file-in-githubs-blame-view/3256 
>>>>
>>>>>>
>>>>>> On Tue, Oct 6, 2020 at 6:00 PM Aljoscha Krettek
>>>>>> <[hidden email]>
>>>> wrote:
>>>>>>
>>>>>>> Maybe I wasn't very clear on how the ratchet works. The changes are
>>>>>>> gradual yes, but it doesn't leave you an option: if you touch a
>>>>>>> file
>>>> you
>>>>>>> will it will have to conform to the coding style afterwards.
>>>>>>> It's not
>>>>>>> like the previous gradual process we had before where it would be
>>>> based
>>>>>>> on people actively working towards a style.
>>>>>>>
>>>>>>> That being said, I also completely like the option of just doing
>>>>>>> one
>>>> big
>>>>>>> change commit.
>>>>>>>
>>>>>>> Regarding actual coding styles: we're a bit limited by what tools
>>>> exist.
>>>>>>> I like Spotless because it can be used to both check and apply a
>>>> style.
>>>>>>> Then you need a formatter that works with Spotless and of those we
>>>> only
>>>>>>> have the Eclipse Formatter, google-java-format, and Prettier.
>>>>>>> Prettier
>>>>>>> is a Javascript tool that I would like to avoid. Eclipse is
>>>>>>> doable but
>>>>>>> you need to fiddle with configuration files. I like
>>>>>>> google-java-format
>>>>>>> because of it's take-it-or-leave-it approach. You either use the
>>>>>>> style
>>>>>>> or you don't but it's very thorough. The downside is that it
>>>>>>> won't do
>>>>>>> tabs-only formatting.
>>>>>>>
>>>>>>> Best,
>>>>>>> Aljoscha
>>>>>>>
>>>>>>> On 06.10.20 17:43, Arvid Heise wrote:
>>>>>>>> After having written that I did a quick search, you can even
>>>>>>>> use git
>>>>>>> blame
>>>>>>>> with one big massive change commit [1], which would further
>>>>>>>> help the
>>>> idea
>>>>>>>> of "just get over with it".
>>>>>>>>
>>>>>>>> With that option, I'd even change all whitespaces if the community
>>>> thinks
>>>>>>>> that it's a better option (a separate discussion that I'll gladly
>>>> skip).
>>>>>>>>
>>>>>>>> [1]
>>>>>>>>
>>>>>>>
>>>> https://www.moxio.com/blog/43/ignoring-bulk-change-commits-with-git-blame 
>>>>
>>>>>>>> On Tue, Oct 6, 2020 at 5:38 PM Arvid Heise <[hidden email]>
>>>> wrote:
>>>>>>>>
>>>>>>>>> I'm also +1 for automatically enforceable code style.
>>>>>>>>>
>>>>>>>>> I also would just go over it as Chesnay said. While it makes some
>>>>>>> changes
>>>>>>>>> a bit harder to track (inline git blame), it's easy to skip
>>>>>>>>> over in
>>>> any
>>>>>>> git
>>>>>>>>> history and if it's only one massive commit, then it's also much
>>>> easier
>>>>>>> to
>>>>>>>>> ignore than many gradual changes. Further, if we just do it once,
>>>> git
>>>>>>> blame
>>>>>>>>> will quickly become more reliable again.
>>>>>>>>>
>>>>>>>>> Btw I completely don't care about the code style as long as it
>>>>>>>>> plays
>>>>>>> well
>>>>>>>>> with IntelliJ (it used to be different, but things change :p).
>>>>>>>>>
>>>>>>>>> On Tue, Oct 6, 2020 at 5:23 PM Chesnay Schepler
>>>>>>>>> <[hidden email]
>>>>>
>>>>>>>>> wrote:
>>>>>>>>>
>>>>>>>>>> We shouldn't switch to spaces _now_; cutting this bit from your
>>>>>>> proposal
>>>>>>>>>> will massively simplify things and there's hardly any value in
>>>> changing
>>>>>>>>>> it.
>>>>>>>>>>
>>>>>>>>>> Also I'm getting rather tired of this constant idea of "gradual
>>>>>>>>>> application". We've been doing this for 2-3 years now since we
>>>>>>>>>> introduced Checkstyle and basically got nowhere. We should just
>>>> bite
>>>>>>> the
>>>>>>>>>> bullet and get it over with; we could've solved this whole
>>>>>>>>>> problem
>>>>>>>>>> already.
>>>>>>>>>>
>>>>>>>>>> In conclusion, I'm +1 on finally locking down the codestyle and
>>>>>>> applying
>>>>>>>>>> it immediately, I'm -1 on any gradual application scheme because
>>>> they
>>>>>>>>>> _just don't work_.
>>>>>>>>>>
>>>>>>>>>> On 10/6/2020 2:15 PM, Aljoscha Krettek wrote:
>>>>>>>>>>> Hi All,
>>>>>>>>>>>
>>>>>>>>>>> I know I know, but please keep reading because I recently
>>>>>>>>>>> learned
>>>>>>>>>>> about some new developments in the area of coding-style
>>>> automation.
>>>>>>>>>>>
>>>>>>>>>>> The tool I would propose we use is Spotless
>>>>>>>>>>> (https://github.com/diffplug/spotless). This doesn't come
>>>>>>>>>>> with a
>>>>>>>>>>> formatter but allows using other popular formatters such as
>>>>>>>>>>> google-java-format. The nice thing about Spotless is that it
>>>> serves as
>>>>>>>>>>> a verifier for CI but can also apply the configured style
>>>>>>>>>>> automatically. That is, for the programmer all she has to do is
>>>> `mvn
>>>>>>>>>>> spotless:apply` to fix any style violations.
>>>>>>>>>>>
>>>>>>>>>>> An interesting feature, which was (somewhat) recently added is
>>>>>>>>>>> "ratchet"
>>>>>>>>>>> (
>>>>>>>
>>>> https://github.com/diffplug/spotless/blob/main/plugin-maven/README.md#ratchet 
>>>>
>>>>>>> ).
>>>>>>>>>>> With this, you can set up Spotless to only apply it's rules to
>>>> files
>>>>>>>>>>> that were changed after a configured commit. This would allow a
>>>>>>>>>>> gradual application of the new coding style instead of one big
>>>> change.
>>>>>>>>>>>
>>>>>>>>>>> If we decide to use Spotless, we would of course also have to
>>>> decide
>>>>>>>>>>> on a coding style. For this I would propose google-java-format,
>>>> which
>>>>>>>>>>> the flink-statefun project uses. The main difference from our
>>>> current
>>>>>>>>>>> "style" is that this uses spaces instead of tabs for
>>>>>>>>>>> indentation.
>>>> By
>>>>>>>>>>> default it would be 2 spaces but it can be configured to use 4
>>>> spaces
>>>>>>>>>>> which would make code look more or less like our current style.
>>>> There
>>>>>>>>>>> are no more configuration knobs, so using tabs is not an
>>>>>>>>>>> option.
>>>>>>>>>>>
>>>>>>>>>>> Finally, why should we do this? I think most engineers agree
>>>>>>>>>>> that
>>>>>>>>>>> having a common enforced style is good to have so I only
>>>>>>>>>>> want to
>>>>>>>>>>> highlight a few thoughts here about things we could improve:
>>>>>>>>>>>
>>>>>>>>>>>     - No more nits about coding style in reviews, this makes it
>>>> easier
>>>>>>>>>>> for both the reviewer and developer
>>>>>>>>>>>
>>>>>>>>>>>     - No manual fixing of Checkstyle errors because Spotless
>>>>>>>>>>> can
>>>> do that
>>>>>>>>>>> automatically
>>>>>>>>>>>
>>>>>>>>>>>     - Because Flink is such a big project little islands of
>>>>>>>>>>> coding
>>>> style
>>>>>>>>>>> have formed between people that commonly work on components. It
>>>> can be
>>>>>>>>>>> a nuisance when you work on a different component and then
>>>> reviewers
>>>>>>>>>>> don't like your typical coding style. And you first have to get
>>>> used
>>>>>>>>>>> to the slight differences in style when reading code.
>>>>>>>>>>>
>>>>>>>>>>> There are also downsides I see in this:
>>>>>>>>>>>
>>>>>>>>>>>     - We break the history, but both "git blame" and modern
>>>> IntelliJ can
>>>>>>>>>>> ignore whitespace when attributing changes. So for files
>>>>>>>>>>> that are
>>>>>>>>>>> already "well" formatted not much would change.
>>>>>>>>>>>
>>>>>>>>>>>     - In the short-term it will be harder to apply changes
>>>>>>>>>>> both to
>>>>>>> master
>>>>>>>>>>> and one of the release-x branches because formatting will be
>>>>>>>>>>> different. I think this is not too hard though because Spotless
>>>> can
>>>>>>>>>>> automatically apply the style.
>>>>>>>>>>>
>>>>>>>>>>> In summary, we would have some short-term pain with this but I
>>>> think
>>>>>>>>>>> it would be good in the long run. What are your thoughts?
>>>>>>>>>>>
>>>>>>>>>>> Best,
>>>>>>>>>>> Aljoscha
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>> --
>>>>>>>>>
>>>>>>>>> Arvid Heise | Senior Java Developer
>>>>>>>>>
>>>>>>>>> <https://www.ververica.com/>
>>>>>>>>>
>>>>>>>>> Follow us @VervericaData
>>>>>>>>>
>>>>>>>>> --
>>>>>>>>>
>>>>>>>>> Join Flink Forward <https://flink-forward.org/> - The Apache
>>>>>>>>> Flink
>>>>>>>>> Conference
>>>>>>>>>
>>>>>>>>> Stream Processing | Event Driven | Real Time
>>>>>>>>>
>>>>>>>>> --
>>>>>>>>>
>>>>>>>>> Ververica GmbH | Invalidenstrasse 115, 10115 Berlin, Germany
>>>>>>>>>
>>>>>>>>> --
>>>>>>>>> Ververica GmbH
>>>>>>>>> Registered at Amtsgericht Charlottenburg: HRB 158244 B
>>>>>>>>> Managing Directors: Timothy Alexander Steinert, Yip Park Tung
>>>> Jason, Ji
>>>>>>>>> (Toni) Cheng
>>>>>>>>>
>>>>>>>>
>>>>>
>>>>
>>>
>>
>
>

[Robert Metzger]

+1

Thanks for driving this.

On Wed, Dec 16, 2020 at 7:33 PM Chesnay Schepler <[hidden email]> wrote:

> +1 to set this up ASAP. Now's a good time to (finally) finalize this
> effort with a new release cycle having started and christmas/vacations
> being around the corner.
>
> On 12/16/2020 7:20 PM, Aljoscha Krettek wrote:
> > Let's try and conclude this discussion! I've prepared a PoC that uses
> > Spotless with google-java-format to do the formatting:
> > https://github.com/aljoscha/flink/commits/flink-xxx-spotless
> >
> > To summarize from earlier discussion, the main benefits are:
> >  - no more worrying about code style, both as reviewer and implementer
> >  - everyone can configure their IDE to auto-format, there will never
> > be unrelated formatting changes
> >
> > Also, this uses git's blame.ignoreRevsFile to add a file that tells
> > git blame/IntelliJ to ignore the refactoring commit. However, you need
> > to manually configure your git for that using:
> >
> > $ git config blame.ignoreRevsFile .git-blame-ignore-revs
> >
> > You can check out the PoC, look at the code in an IDE, play around
> > with blame/annotations.
> >
> > By the way, the coding style is not configurable, it’s the Google Java
> > Style, wich uses spaces for indentation. In an IDE or on github the
> > code looks barely different from the previous formatting at a first
> > glance.
> >
> > For IDE setup, I will add a guide in the README but it boils down to:
> >
> >  - install the google-java-format plugin, enable it
> >  - install the Save Actions plugin, enable "Optimize Imports" and
> > "Reformat File"
> >
> > With this, every time you save, formatting will be applied
> > automatically. You will never see formatting complaints from CI
> > (except for cases where you break semantical checkstyle rules, such as
> > using certain imports that we don't allow.).
> >
> > What do you think?
> >
> > Best,
> > Aljoscha
> >
> > On 19.10.20 12:36, Aljoscha Krettek wrote:
> >> I don't like checkstyle because it cannot be easily applied from the
> >> commandline. I'm happy to learn otherwise, though. And I'd also be
> >> very happy about alternative suggestions that can do that.
> >>
> >> Right now, I think Spotless is the most straightforward tool for
> >> that. Also I don't care so much about what style we choose in the
> >> end. (If we choose one). My main motivation is that we have a common,
> >> strict style that can easily applied via tooling so that we no longer
> >> need to comment on coding style in PRs.
> >>
> >> Aljoscha
> >>
> >> On 09.10.20 11:11, tison wrote:
> >>> +1 on locking on one codestyle and I think that is what current
> >>> checkstyle
> >>> rules serving.
> >>>
> >>> For automatic applying part, we suggest developing by IDEA and with
> >>> Checkstyle Plugin on IDEA applying checkstyle suggestion is also
> >>> automatic.
> >>>
> >>> One short coming for spotless is that we can hardly adjust rules if the
> >>> project has its own issues to overcome. IIRC only several
> >>> pre-defined rules
> >>> and a clumsy general regex substitution rule can be used.
> >>>
> >>> FYI my team started with spotless but ended up with checkstyle with few
> >>> rules and Checkstyle Plugin for automation.
> >>>
> >>> Codestyle, in another perspective, is part of cognition of developers
> >>> working in project, not something we just apply before pull request. No
> >>> matter how much automation introduced, most of developers will converge
> >>> working with the configured codestyle.
> >>>
> >>> Best,
> >>> tison.
> >>>
> >>>
> >>> Kostas Kloudas <[hidden email]> 于2020年10月7日周三 下午6:37写道：
> >>>
> >>>> Hi all,
> >>>>
> >>>> +1 for enforcing "a" codestyle using "a" tool.
> >>>>
> >>>> As the project grows both in terms of LOCs and contributors, this
> >>>> becomes more and more important as it eliminates some potential points
> >>>> of friction without any additional effort.
> >>>>
> >>>>  From the discussion, I am leaning towards having a single commit with
> >>>> all the codestyle-related changes. This will avoid sprinkling
> >>>> refactoring commits all over the place for the next year or more. But
> >>>> if this is the price to pay for having consensus on a tool, then I am
> >>>> ok with gradual implementation. I believe that the value added by
> >>>> having an automated process of enforcing a codestyle exceeds the cost
> >>>> of the nuisance of gradual refactoring.
> >>>>
> >>>> As for the actual format, I like the google-java-format but again, if
> >>>> the community agrees on a different one I would not oppose that (as
> >>>> long as it does not use the same amount of indentation for method args
> >>>> and method body :P).
> >>>>
> >>>> Cheers,
> >>>> Kostas
> >>>>
> >>>> On Wed, Oct 7, 2020 at 10:26 AM Chesnay Schepler <[hidden email]>
> >>>> wrote:
> >>>>>
> >>>>> To me, ratchet seems to combine the worst aspects of both approaches.
> >>>>> You get disruptive changes, but only in singular files,
> >>>>> for something mundane as a typo fix or import change, which would be
> >>>>> annoying to keep separate from the actual functional changes in a PR.
> >>>>>
> >>>>> On 10/7/2020 10:04 AM, Matthias Pohl wrote:
> >>>>>> I find the ratchet feature you're suggesting interesting. But Arvid
> >>>> has a
> >>>>>> point referring to the blog post about ignoring revisions in git
> >>>>>> blame
> >>>> [1].
> >>>>>> Adding the configuration file for commits to ignore revs as proposed
> >>>> in the
> >>>>>> blog post makes it even easier. One problem I see is that this is
> >>>>>> not
> >>>>>> supported by Github (yet?) [2] as mentioned in [1].
> >>>>>>
> >>>>>> Considering all that I prefer applying the code style in one go. I
> >>>> have no
> >>>>>> strong opinion on what codestyle is the best.
> >>>>>>
> >>>>>> PS: We used spotless in the project I previously worked on. It was
> >>>>>> convenient to use.
> >>>>>>
> >>>>>> [1]
> >>>>>>
> >>>>
> https://www.moxio.com/blog/43/ignoring-bulk-change-commits-with-git-blame
> >>>>
> >>>>>> [2]
> >>>>>>
> >>>>
> https://github.community/t/support-ignore-revs-file-in-githubs-blame-view/3256
> >>>>
> >>>>>>
> >>>>>> On Tue, Oct 6, 2020 at 6:00 PM Aljoscha Krettek
> >>>>>> <[hidden email]>
> >>>> wrote:
> >>>>>>
> >>>>>>> Maybe I wasn't very clear on how the ratchet works. The changes are
> >>>>>>> gradual yes, but it doesn't leave you an option: if you touch a
> >>>>>>> file
> >>>> you
> >>>>>>> will it will have to conform to the coding style afterwards.
> >>>>>>> It's not
> >>>>>>> like the previous gradual process we had before where it would be
> >>>> based
> >>>>>>> on people actively working towards a style.
> >>>>>>>
> >>>>>>> That being said, I also completely like the option of just doing
> >>>>>>> one
> >>>> big
> >>>>>>> change commit.
> >>>>>>>
> >>>>>>> Regarding actual coding styles: we're a bit limited by what tools
> >>>> exist.
> >>>>>>> I like Spotless because it can be used to both check and apply a
> >>>> style.
> >>>>>>> Then you need a formatter that works with Spotless and of those we
> >>>> only
> >>>>>>> have the Eclipse Formatter, google-java-format, and Prettier.
> >>>>>>> Prettier
> >>>>>>> is a Javascript tool that I would like to avoid. Eclipse is
> >>>>>>> doable but
> >>>>>>> you need to fiddle with configuration files. I like
> >>>>>>> google-java-format
> >>>>>>> because of it's take-it-or-leave-it approach. You either use the
> >>>>>>> style
> >>>>>>> or you don't but it's very thorough. The downside is that it
> >>>>>>> won't do
> >>>>>>> tabs-only formatting.
> >>>>>>>
> >>>>>>> Best,
> >>>>>>> Aljoscha
> >>>>>>>
> >>>>>>> On 06.10.20 17:43, Arvid Heise wrote:
> >>>>>>>> After having written that I did a quick search, you can even
> >>>>>>>> use git
> >>>>>>> blame
> >>>>>>>> with one big massive change commit [1], which would further
> >>>>>>>> help the
> >>>> idea
> >>>>>>>> of "just get over with it".
> >>>>>>>>
> >>>>>>>> With that option, I'd even change all whitespaces if the community
> >>>> thinks
> >>>>>>>> that it's a better option (a separate discussion that I'll gladly
> >>>> skip).
> >>>>>>>>
> >>>>>>>> [1]
> >>>>>>>>
> >>>>>>>
> >>>>
> https://www.moxio.com/blog/43/ignoring-bulk-change-commits-with-git-blame
> >>>>
> >>>>>>>> On Tue, Oct 6, 2020 at 5:38 PM Arvid Heise <[hidden email]>
> >>>> wrote:
> >>>>>>>>
> >>>>>>>>> I'm also +1 for automatically enforceable code style.
> >>>>>>>>>
> >>>>>>>>> I also would just go over it as Chesnay said. While it makes some
> >>>>>>> changes
> >>>>>>>>> a bit harder to track (inline git blame), it's easy to skip
> >>>>>>>>> over in
> >>>> any
> >>>>>>> git
> >>>>>>>>> history and if it's only one massive commit, then it's also much
> >>>> easier
> >>>>>>> to
> >>>>>>>>> ignore than many gradual changes. Further, if we just do it once,
> >>>> git
> >>>>>>> blame
> >>>>>>>>> will quickly become more reliable again.
> >>>>>>>>>
> >>>>>>>>> Btw I completely don't care about the code style as long as it
> >>>>>>>>> plays
> >>>>>>> well
> >>>>>>>>> with IntelliJ (it used to be different, but things change :p).
> >>>>>>>>>
> >>>>>>>>> On Tue, Oct 6, 2020 at 5:23 PM Chesnay Schepler
> >>>>>>>>> <[hidden email]
> >>>>>
> >>>>>>>>> wrote:
> >>>>>>>>>
> >>>>>>>>>> We shouldn't switch to spaces _now_; cutting this bit from your
> >>>>>>> proposal
> >>>>>>>>>> will massively simplify things and there's hardly any value in
> >>>> changing
> >>>>>>>>>> it.
> >>>>>>>>>>
> >>>>>>>>>> Also I'm getting rather tired of this constant idea of "gradual
> >>>>>>>>>> application". We've been doing this for 2-3 years now since we
> >>>>>>>>>> introduced Checkstyle and basically got nowhere. We should just
> >>>> bite
> >>>>>>> the
> >>>>>>>>>> bullet and get it over with; we could've solved this whole
> >>>>>>>>>> problem
> >>>>>>>>>> already.
> >>>>>>>>>>
> >>>>>>>>>> In conclusion, I'm +1 on finally locking down the codestyle and
> >>>>>>> applying
> >>>>>>>>>> it immediately, I'm -1 on any gradual application scheme because
> >>>> they
> >>>>>>>>>> _just don't work_.
> >>>>>>>>>>
> >>>>>>>>>> On 10/6/2020 2:15 PM, Aljoscha Krettek wrote:
> >>>>>>>>>>> Hi All,
> >>>>>>>>>>>
> >>>>>>>>>>> I know I know, but please keep reading because I recently
> >>>>>>>>>>> learned
> >>>>>>>>>>> about some new developments in the area of coding-style
> >>>> automation.
> >>>>>>>>>>>
> >>>>>>>>>>> The tool I would propose we use is Spotless
> >>>>>>>>>>> (https://github.com/diffplug/spotless). This doesn't come
> >>>>>>>>>>> with a
> >>>>>>>>>>> formatter but allows using other popular formatters such as
> >>>>>>>>>>> google-java-format. The nice thing about Spotless is that it
> >>>> serves as
> >>>>>>>>>>> a verifier for CI but can also apply the configured style
> >>>>>>>>>>> automatically. That is, for the programmer all she has to do is
> >>>> `mvn
> >>>>>>>>>>> spotless:apply` to fix any style violations.
> >>>>>>>>>>>
> >>>>>>>>>>> An interesting feature, which was (somewhat) recently added is
> >>>>>>>>>>> "ratchet"
> >>>>>>>>>>> (
> >>>>>>>
> >>>>
> https://github.com/diffplug/spotless/blob/main/plugin-maven/README.md#ratchet
> >>>>
> >>>>>>> ).
> >>>>>>>>>>> With this, you can set up Spotless to only apply it's rules to
> >>>> files
> >>>>>>>>>>> that were changed after a configured commit. This would allow a
> >>>>>>>>>>> gradual application of the new coding style instead of one big
> >>>> change.
> >>>>>>>>>>>
> >>>>>>>>>>> If we decide to use Spotless, we would of course also have to
> >>>> decide
> >>>>>>>>>>> on a coding style. For this I would propose google-java-format,
> >>>> which
> >>>>>>>>>>> the flink-statefun project uses. The main difference from our
> >>>> current
> >>>>>>>>>>> "style" is that this uses spaces instead of tabs for
> >>>>>>>>>>> indentation.
> >>>> By
> >>>>>>>>>>> default it would be 2 spaces but it can be configured to use 4
> >>>> spaces
> >>>>>>>>>>> which would make code look more or less like our current style.
> >>>> There
> >>>>>>>>>>> are no more configuration knobs, so using tabs is not an
> >>>>>>>>>>> option.
> >>>>>>>>>>>
> >>>>>>>>>>> Finally, why should we do this? I think most engineers agree
> >>>>>>>>>>> that
> >>>>>>>>>>> having a common enforced style is good to have so I only
> >>>>>>>>>>> want to
> >>>>>>>>>>> highlight a few thoughts here about things we could improve:
> >>>>>>>>>>>
> >>>>>>>>>>>     - No more nits about coding style in reviews, this makes it
> >>>> easier
> >>>>>>>>>>> for both the reviewer and developer
> >>>>>>>>>>>
> >>>>>>>>>>>     - No manual fixing of Checkstyle errors because Spotless
> >>>>>>>>>>> can
> >>>> do that
> >>>>>>>>>>> automatically
> >>>>>>>>>>>
> >>>>>>>>>>>     - Because Flink is such a big project little islands of
> >>>>>>>>>>> coding
> >>>> style
> >>>>>>>>>>> have formed between people that commonly work on components. It
> >>>> can be
> >>>>>>>>>>> a nuisance when you work on a different component and then
> >>>> reviewers
> >>>>>>>>>>> don't like your typical coding style. And you first have to get
> >>>> used
> >>>>>>>>>>> to the slight differences in style when reading code.
> >>>>>>>>>>>
> >>>>>>>>>>> There are also downsides I see in this:
> >>>>>>>>>>>
> >>>>>>>>>>>     - We break the history, but both "git blame" and modern
> >>>> IntelliJ can
> >>>>>>>>>>> ignore whitespace when attributing changes. So for files
> >>>>>>>>>>> that are
> >>>>>>>>>>> already "well" formatted not much would change.
> >>>>>>>>>>>
> >>>>>>>>>>>     - In the short-term it will be harder to apply changes
> >>>>>>>>>>> both to
> >>>>>>> master
> >>>>>>>>>>> and one of the release-x branches because formatting will be
> >>>>>>>>>>> different. I think this is not too hard though because Spotless
> >>>> can
> >>>>>>>>>>> automatically apply the style.
> >>>>>>>>>>>
> >>>>>>>>>>> In summary, we would have some short-term pain with this but I
> >>>> think
> >>>>>>>>>>> it would be good in the long run. What are your thoughts?
> >>>>>>>>>>>
> >>>>>>>>>>> Best,
> >>>>>>>>>>> Aljoscha
> >>>>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>> --
> >>>>>>>>>
> >>>>>>>>> Arvid Heise | Senior Java Developer
> >>>>>>>>>
> >>>>>>>>> <https://www.ververica.com/>
> >>>>>>>>>
> >>>>>>>>> Follow us @VervericaData
> >>>>>>>>>
> >>>>>>>>> --
> >>>>>>>>>
> >>>>>>>>> Join Flink Forward <https://flink-forward.org/> - The Apache
> >>>>>>>>> Flink
> >>>>>>>>> Conference
> >>>>>>>>>
> >>>>>>>>> Stream Processing | Event Driven | Real Time
> >>>>>>>>>
> >>>>>>>>> --
> >>>>>>>>>
> >>>>>>>>> Ververica GmbH | Invalidenstrasse 115, 10115 Berlin, Germany
> >>>>>>>>>
> >>>>>>>>> --
> >>>>>>>>> Ververica GmbH
> >>>>>>>>> Registered at Amtsgericht Charlottenburg: HRB 158244 B
> >>>>>>>>> Managing Directors: Timothy Alexander Steinert, Yip Park Tung
> >>>> Jason, Ji
> >>>>>>>>> (Toni) Cheng
> >>>>>>>>>
> >>>>>>>>
> >>>>>
> >>>>
> >>>
> >>
> >
> >
>
>

[Thomas Weise]

+1 for better coding style automation

I see spotless works very well in other projects.

On Wed, Dec 16, 2020 at 10:45 AM Robert Metzger <[hidden email]> wrote:

> +1
>
> Thanks for driving this.
>
> On Wed, Dec 16, 2020 at 7:33 PM Chesnay Schepler <[hidden email]>
> wrote:
>
> > +1 to set this up ASAP. Now's a good time to (finally) finalize this
> > effort with a new release cycle having started and christmas/vacations
> > being around the corner.
> >
> > On 12/16/2020 7:20 PM, Aljoscha Krettek wrote:
> > > Let's try and conclude this discussion! I've prepared a PoC that uses
> > > Spotless with google-java-format to do the formatting:
> > > https://github.com/aljoscha/flink/commits/flink-xxx-spotless
> > >
> > > To summarize from earlier discussion, the main benefits are:
> > >  - no more worrying about code style, both as reviewer and implementer
> > >  - everyone can configure their IDE to auto-format, there will never
> > > be unrelated formatting changes
> > >
> > > Also, this uses git's blame.ignoreRevsFile to add a file that tells
> > > git blame/IntelliJ to ignore the refactoring commit. However, you need
> > > to manually configure your git for that using:
> > >
> > > $ git config blame.ignoreRevsFile .git-blame-ignore-revs
> > >
> > > You can check out the PoC, look at the code in an IDE, play around
> > > with blame/annotations.
> > >
> > > By the way, the coding style is not configurable, it’s the Google Java
> > > Style, wich uses spaces for indentation. In an IDE or on github the
> > > code looks barely different from the previous formatting at a first
> > > glance.
> > >
> > > For IDE setup, I will add a guide in the README but it boils down to:
> > >
> > >  - install the google-java-format plugin, enable it
> > >  - install the Save Actions plugin, enable "Optimize Imports" and
> > > "Reformat File"
> > >
> > > With this, every time you save, formatting will be applied
> > > automatically. You will never see formatting complaints from CI
> > > (except for cases where you break semantical checkstyle rules, such as
> > > using certain imports that we don't allow.).
> > >
> > > What do you think?
> > >
> > > Best,
> > > Aljoscha
> > >
> > > On 19.10.20 12:36, Aljoscha Krettek wrote:
> > >> I don't like checkstyle because it cannot be easily applied from the
> > >> commandline. I'm happy to learn otherwise, though. And I'd also be
> > >> very happy about alternative suggestions that can do that.
> > >>
> > >> Right now, I think Spotless is the most straightforward tool for
> > >> that. Also I don't care so much about what style we choose in the
> > >> end. (If we choose one). My main motivation is that we have a common,
> > >> strict style that can easily applied via tooling so that we no longer
> > >> need to comment on coding style in PRs.
> > >>
> > >> Aljoscha
> > >>
> > >> On 09.10.20 11:11, tison wrote:
> > >>> +1 on locking on one codestyle and I think that is what current
> > >>> checkstyle
> > >>> rules serving.
> > >>>
> > >>> For automatic applying part, we suggest developing by IDEA and with
> > >>> Checkstyle Plugin on IDEA applying checkstyle suggestion is also
> > >>> automatic.
> > >>>
> > >>> One short coming for spotless is that we can hardly adjust rules if
> the
> > >>> project has its own issues to overcome. IIRC only several
> > >>> pre-defined rules
> > >>> and a clumsy general regex substitution rule can be used.
> > >>>
> > >>> FYI my team started with spotless but ended up with checkstyle with
> few
> > >>> rules and Checkstyle Plugin for automation.
> > >>>
> > >>> Codestyle, in another perspective, is part of cognition of developers
> > >>> working in project, not something we just apply before pull request.
> No
> > >>> matter how much automation introduced, most of developers will
> converge
> > >>> working with the configured codestyle.
> > >>>
> > >>> Best,
> > >>> tison.
> > >>>
> > >>>
> > >>> Kostas Kloudas <[hidden email]> 于2020年10月7日周三 下午6:37写道：
> > >>>
> > >>>> Hi all,
> > >>>>
> > >>>> +1 for enforcing "a" codestyle using "a" tool.
> > >>>>
> > >>>> As the project grows both in terms of LOCs and contributors, this
> > >>>> becomes more and more important as it eliminates some potential
> points
> > >>>> of friction without any additional effort.
> > >>>>
> > >>>>  From the discussion, I am leaning towards having a single commit
> with
> > >>>> all the codestyle-related changes. This will avoid sprinkling
> > >>>> refactoring commits all over the place for the next year or more.
> But
> > >>>> if this is the price to pay for having consensus on a tool, then I
> am
> > >>>> ok with gradual implementation. I believe that the value added by
> > >>>> having an automated process of enforcing a codestyle exceeds the
> cost
> > >>>> of the nuisance of gradual refactoring.
> > >>>>
> > >>>> As for the actual format, I like the google-java-format but again,
> if
> > >>>> the community agrees on a different one I would not oppose that (as
> > >>>> long as it does not use the same amount of indentation for method
> args
> > >>>> and method body :P).
> > >>>>
> > >>>> Cheers,
> > >>>> Kostas
> > >>>>
> > >>>> On Wed, Oct 7, 2020 at 10:26 AM Chesnay Schepler <
> [hidden email]>
> > >>>> wrote:
> > >>>>>
> > >>>>> To me, ratchet seems to combine the worst aspects of both
> approaches.
> > >>>>> You get disruptive changes, but only in singular files,
> > >>>>> for something mundane as a typo fix or import change, which would
> be
> > >>>>> annoying to keep separate from the actual functional changes in a
> PR.
> > >>>>>
> > >>>>> On 10/7/2020 10:04 AM, Matthias Pohl wrote:
> > >>>>>> I find the ratchet feature you're suggesting interesting. But
> Arvid
> > >>>> has a
> > >>>>>> point referring to the blog post about ignoring revisions in git
> > >>>>>> blame
> > >>>> [1].
> > >>>>>> Adding the configuration file for commits to ignore revs as
> proposed
> > >>>> in the
> > >>>>>> blog post makes it even easier. One problem I see is that this is
> > >>>>>> not
> > >>>>>> supported by Github (yet?) [2] as mentioned in [1].
> > >>>>>>
> > >>>>>> Considering all that I prefer applying the code style in one go. I
> > >>>> have no
> > >>>>>> strong opinion on what codestyle is the best.
> > >>>>>>
> > >>>>>> PS: We used spotless in the project I previously worked on. It was
> > >>>>>> convenient to use.
> > >>>>>>
> > >>>>>> [1]
> > >>>>>>
> > >>>>
> >
> https://www.moxio.com/blog/43/ignoring-bulk-change-commits-with-git-blame
> > >>>>
> > >>>>>> [2]
> > >>>>>>
> > >>>>
> >
> https://github.community/t/support-ignore-revs-file-in-githubs-blame-view/3256
> > >>>>
> > >>>>>>
> > >>>>>> On Tue, Oct 6, 2020 at 6:00 PM Aljoscha Krettek
> > >>>>>> <[hidden email]>
> > >>>> wrote:
> > >>>>>>
> > >>>>>>> Maybe I wasn't very clear on how the ratchet works. The changes
> are
> > >>>>>>> gradual yes, but it doesn't leave you an option: if you touch a
> > >>>>>>> file
> > >>>> you
> > >>>>>>> will it will have to conform to the coding style afterwards.
> > >>>>>>> It's not
> > >>>>>>> like the previous gradual process we had before where it would be
> > >>>> based
> > >>>>>>> on people actively working towards a style.
> > >>>>>>>
> > >>>>>>> That being said, I also completely like the option of just doing
> > >>>>>>> one
> > >>>> big
> > >>>>>>> change commit.
> > >>>>>>>
> > >>>>>>> Regarding actual coding styles: we're a bit limited by what tools
> > >>>> exist.
> > >>>>>>> I like Spotless because it can be used to both check and apply a
> > >>>> style.
> > >>>>>>> Then you need a formatter that works with Spotless and of those
> we
> > >>>> only
> > >>>>>>> have the Eclipse Formatter, google-java-format, and Prettier.
> > >>>>>>> Prettier
> > >>>>>>> is a Javascript tool that I would like to avoid. Eclipse is
> > >>>>>>> doable but
> > >>>>>>> you need to fiddle with configuration files. I like
> > >>>>>>> google-java-format
> > >>>>>>> because of it's take-it-or-leave-it approach. You either use the
> > >>>>>>> style
> > >>>>>>> or you don't but it's very thorough. The downside is that it
> > >>>>>>> won't do
> > >>>>>>> tabs-only formatting.
> > >>>>>>>
> > >>>>>>> Best,
> > >>>>>>> Aljoscha
> > >>>>>>>
> > >>>>>>> On 06.10.20 17:43, Arvid Heise wrote:
> > >>>>>>>> After having written that I did a quick search, you can even
> > >>>>>>>> use git
> > >>>>>>> blame
> > >>>>>>>> with one big massive change commit [1], which would further
> > >>>>>>>> help the
> > >>>> idea
> > >>>>>>>> of "just get over with it".
> > >>>>>>>>
> > >>>>>>>> With that option, I'd even change all whitespaces if the
> community
> > >>>> thinks
> > >>>>>>>> that it's a better option (a separate discussion that I'll
> gladly
> > >>>> skip).
> > >>>>>>>>
> > >>>>>>>> [1]
> > >>>>>>>>
> > >>>>>>>
> > >>>>
> >
> https://www.moxio.com/blog/43/ignoring-bulk-change-commits-with-git-blame
> > >>>>
> > >>>>>>>> On Tue, Oct 6, 2020 at 5:38 PM Arvid Heise <[hidden email]
> >
> > >>>> wrote:
> > >>>>>>>>
> > >>>>>>>>> I'm also +1 for automatically enforceable code style.
> > >>>>>>>>>
> > >>>>>>>>> I also would just go over it as Chesnay said. While it makes
> some
> > >>>>>>> changes
> > >>>>>>>>> a bit harder to track (inline git blame), it's easy to skip
> > >>>>>>>>> over in
> > >>>> any
> > >>>>>>> git
> > >>>>>>>>> history and if it's only one massive commit, then it's also
> much
> > >>>> easier
> > >>>>>>> to
> > >>>>>>>>> ignore than many gradual changes. Further, if we just do it
> once,
> > >>>> git
> > >>>>>>> blame
> > >>>>>>>>> will quickly become more reliable again.
> > >>>>>>>>>
> > >>>>>>>>> Btw I completely don't care about the code style as long as it
> > >>>>>>>>> plays
> > >>>>>>> well
> > >>>>>>>>> with IntelliJ (it used to be different, but things change :p).
> > >>>>>>>>>
> > >>>>>>>>> On Tue, Oct 6, 2020 at 5:23 PM Chesnay Schepler
> > >>>>>>>>> <[hidden email]
> > >>>>>
> > >>>>>>>>> wrote:
> > >>>>>>>>>
> > >>>>>>>>>> We shouldn't switch to spaces _now_; cutting this bit from
> your
> > >>>>>>> proposal
> > >>>>>>>>>> will massively simplify things and there's hardly any value in
> > >>>> changing
> > >>>>>>>>>> it.
> > >>>>>>>>>>
> > >>>>>>>>>> Also I'm getting rather tired of this constant idea of
> "gradual
> > >>>>>>>>>> application". We've been doing this for 2-3 years now since we
> > >>>>>>>>>> introduced Checkstyle and basically got nowhere. We should
> just
> > >>>> bite
> > >>>>>>> the
> > >>>>>>>>>> bullet and get it over with; we could've solved this whole
> > >>>>>>>>>> problem
> > >>>>>>>>>> already.
> > >>>>>>>>>>
> > >>>>>>>>>> In conclusion, I'm +1 on finally locking down the codestyle
> and
> > >>>>>>> applying
> > >>>>>>>>>> it immediately, I'm -1 on any gradual application scheme
> because
> > >>>> they
> > >>>>>>>>>> _just don't work_.
> > >>>>>>>>>>
> > >>>>>>>>>> On 10/6/2020 2:15 PM, Aljoscha Krettek wrote:
> > >>>>>>>>>>> Hi All,
> > >>>>>>>>>>>
> > >>>>>>>>>>> I know I know, but please keep reading because I recently
> > >>>>>>>>>>> learned
> > >>>>>>>>>>> about some new developments in the area of coding-style
> > >>>> automation.
> > >>>>>>>>>>>
> > >>>>>>>>>>> The tool I would propose we use is Spotless
> > >>>>>>>>>>> (https://github.com/diffplug/spotless). This doesn't come
> > >>>>>>>>>>> with a
> > >>>>>>>>>>> formatter but allows using other popular formatters such as
> > >>>>>>>>>>> google-java-format. The nice thing about Spotless is that it
> > >>>> serves as
> > >>>>>>>>>>> a verifier for CI but can also apply the configured style
> > >>>>>>>>>>> automatically. That is, for the programmer all she has to do
> is
> > >>>> `mvn
> > >>>>>>>>>>> spotless:apply` to fix any style violations.
> > >>>>>>>>>>>
> > >>>>>>>>>>> An interesting feature, which was (somewhat) recently added
> is
> > >>>>>>>>>>> "ratchet"
> > >>>>>>>>>>> (
> > >>>>>>>
> > >>>>
> >
> https://github.com/diffplug/spotless/blob/main/plugin-maven/README.md#ratchet
> > >>>>
> > >>>>>>> ).
> > >>>>>>>>>>> With this, you can set up Spotless to only apply it's rules
> to
> > >>>> files
> > >>>>>>>>>>> that were changed after a configured commit. This would
> allow a
> > >>>>>>>>>>> gradual application of the new coding style instead of one
> big
> > >>>> change.
> > >>>>>>>>>>>
> > >>>>>>>>>>> If we decide to use Spotless, we would of course also have to
> > >>>> decide
> > >>>>>>>>>>> on a coding style. For this I would propose
> google-java-format,
> > >>>> which
> > >>>>>>>>>>> the flink-statefun project uses. The main difference from our
> > >>>> current
> > >>>>>>>>>>> "style" is that this uses spaces instead of tabs for
> > >>>>>>>>>>> indentation.
> > >>>> By
> > >>>>>>>>>>> default it would be 2 spaces but it can be configured to use
> 4
> > >>>> spaces
> > >>>>>>>>>>> which would make code look more or less like our current
> style.
> > >>>> There
> > >>>>>>>>>>> are no more configuration knobs, so using tabs is not an
> > >>>>>>>>>>> option.
> > >>>>>>>>>>>
> > >>>>>>>>>>> Finally, why should we do this? I think most engineers agree
> > >>>>>>>>>>> that
> > >>>>>>>>>>> having a common enforced style is good to have so I only
> > >>>>>>>>>>> want to
> > >>>>>>>>>>> highlight a few thoughts here about things we could improve:
> > >>>>>>>>>>>
> > >>>>>>>>>>>     - No more nits about coding style in reviews, this makes
> it
> > >>>> easier
> > >>>>>>>>>>> for both the reviewer and developer
> > >>>>>>>>>>>
> > >>>>>>>>>>>     - No manual fixing of Checkstyle errors because Spotless
> > >>>>>>>>>>> can
> > >>>> do that
> > >>>>>>>>>>> automatically
> > >>>>>>>>>>>
> > >>>>>>>>>>>     - Because Flink is such a big project little islands of
> > >>>>>>>>>>> coding
> > >>>> style
> > >>>>>>>>>>> have formed between people that commonly work on components.
> It
> > >>>> can be
> > >>>>>>>>>>> a nuisance when you work on a different component and then
> > >>>> reviewers
> > >>>>>>>>>>> don't like your typical coding style. And you first have to
> get
> > >>>> used
> > >>>>>>>>>>> to the slight differences in style when reading code.
> > >>>>>>>>>>>
> > >>>>>>>>>>> There are also downsides I see in this:
> > >>>>>>>>>>>
> > >>>>>>>>>>>     - We break the history, but both "git blame" and modern
> > >>>> IntelliJ can
> > >>>>>>>>>>> ignore whitespace when attributing changes. So for files
> > >>>>>>>>>>> that are
> > >>>>>>>>>>> already "well" formatted not much would change.
> > >>>>>>>>>>>
> > >>>>>>>>>>>     - In the short-term it will be harder to apply changes
> > >>>>>>>>>>> both to
> > >>>>>>> master
> > >>>>>>>>>>> and one of the release-x branches because formatting will be
> > >>>>>>>>>>> different. I think this is not too hard though because
> Spotless
> > >>>> can
> > >>>>>>>>>>> automatically apply the style.
> > >>>>>>>>>>>
> > >>>>>>>>>>> In summary, we would have some short-term pain with this but
> I
> > >>>> think
> > >>>>>>>>>>> it would be good in the long run. What are your thoughts?
> > >>>>>>>>>>>
> > >>>>>>>>>>> Best,
> > >>>>>>>>>>> Aljoscha
> > >>>>>>>>>>>
> > >>>>>>>>>>
> > >>>>>>>>> --
> > >>>>>>>>>
> > >>>>>>>>> Arvid Heise | Senior Java Developer
> > >>>>>>>>>
> > >>>>>>>>> <https://www.ververica.com/>
> > >>>>>>>>>
> > >>>>>>>>> Follow us @VervericaData
> > >>>>>>>>>
> > >>>>>>>>> --
> > >>>>>>>>>
> > >>>>>>>>> Join Flink Forward <https://flink-forward.org/> - The Apache
> > >>>>>>>>> Flink
> > >>>>>>>>> Conference
> > >>>>>>>>>
> > >>>>>>>>> Stream Processing | Event Driven | Real Time
> > >>>>>>>>>
> > >>>>>>>>> --
> > >>>>>>>>>
> > >>>>>>>>> Ververica GmbH | Invalidenstrasse 115, 10115 Berlin, Germany
> > >>>>>>>>>
> > >>>>>>>>> --
> > >>>>>>>>> Ververica GmbH
> > >>>>>>>>> Registered at Amtsgericht Charlottenburg: HRB 158244 B
> > >>>>>>>>> Managing Directors: Timothy Alexander Steinert, Yip Park Tung
> > >>>> Jason, Ji
> > >>>>>>>>> (Toni) Cheng
> > >>>>>>>>>
> > >>>>>>>>
> > >>>>>
> > >>>>
> > >>>
> > >>
> > >
> > >
> >
> >
>

[Arvid Heise-3]

+1 asap.

On Wed, Dec 16, 2020 at 7:44 PM Robert Metzger <[hidden email]> wrote:

> +1
>
> Thanks for driving this.
>
> On Wed, Dec 16, 2020 at 7:33 PM Chesnay Schepler <[hidden email]>
> wrote:
>
> > +1 to set this up ASAP. Now's a good time to (finally) finalize this
> > effort with a new release cycle having started and christmas/vacations
> > being around the corner.
> >
> > On 12/16/2020 7:20 PM, Aljoscha Krettek wrote:
> > > Let's try and conclude this discussion! I've prepared a PoC that uses
> > > Spotless with google-java-format to do the formatting:
> > > https://github.com/aljoscha/flink/commits/flink-xxx-spotless
> > >
> > > To summarize from earlier discussion, the main benefits are:
> > >  - no more worrying about code style, both as reviewer and implementer
> > >  - everyone can configure their IDE to auto-format, there will never
> > > be unrelated formatting changes
> > >
> > > Also, this uses git's blame.ignoreRevsFile to add a file that tells
> > > git blame/IntelliJ to ignore the refactoring commit. However, you need
> > > to manually configure your git for that using:
> > >
> > > $ git config blame.ignoreRevsFile .git-blame-ignore-revs
> > >
> > > You can check out the PoC, look at the code in an IDE, play around
> > > with blame/annotations.
> > >
> > > By the way, the coding style is not configurable, it’s the Google Java
> > > Style, wich uses spaces for indentation. In an IDE or on github the
> > > code looks barely different from the previous formatting at a first
> > > glance.
> > >
> > > For IDE setup, I will add a guide in the README but it boils down to:
> > >
> > >  - install the google-java-format plugin, enable it
> > >  - install the Save Actions plugin, enable "Optimize Imports" and
> > > "Reformat File"
> > >
> > > With this, every time you save, formatting will be applied
> > > automatically. You will never see formatting complaints from CI
> > > (except for cases where you break semantical checkstyle rules, such as
> > > using certain imports that we don't allow.).
> > >
> > > What do you think?
> > >
> > > Best,
> > > Aljoscha
> > >
> > > On 19.10.20 12:36, Aljoscha Krettek wrote:
> > >> I don't like checkstyle because it cannot be easily applied from the
> > >> commandline. I'm happy to learn otherwise, though. And I'd also be
> > >> very happy about alternative suggestions that can do that.
> > >>
> > >> Right now, I think Spotless is the most straightforward tool for
> > >> that. Also I don't care so much about what style we choose in the
> > >> end. (If we choose one). My main motivation is that we have a common,
> > >> strict style that can easily applied via tooling so that we no longer
> > >> need to comment on coding style in PRs.
> > >>
> > >> Aljoscha
> > >>
> > >> On 09.10.20 11:11, tison wrote:
> > >>> +1 on locking on one codestyle and I think that is what current
> > >>> checkstyle
> > >>> rules serving.
> > >>>
> > >>> For automatic applying part, we suggest developing by IDEA and with
> > >>> Checkstyle Plugin on IDEA applying checkstyle suggestion is also
> > >>> automatic.
> > >>>
> > >>> One short coming for spotless is that we can hardly adjust rules if
> the
> > >>> project has its own issues to overcome. IIRC only several
> > >>> pre-defined rules
> > >>> and a clumsy general regex substitution rule can be used.
> > >>>
> > >>> FYI my team started with spotless but ended up with checkstyle with
> few
> > >>> rules and Checkstyle Plugin for automation.
> > >>>
> > >>> Codestyle, in another perspective, is part of cognition of developers
> > >>> working in project, not something we just apply before pull request.
> No
> > >>> matter how much automation introduced, most of developers will
> converge
> > >>> working with the configured codestyle.
> > >>>
> > >>> Best,
> > >>> tison.
> > >>>
> > >>>
> > >>> Kostas Kloudas <[hidden email]> 于2020年10月7日周三 下午6:37写道：
> > >>>
> > >>>> Hi all,
> > >>>>
> > >>>> +1 for enforcing "a" codestyle using "a" tool.
> > >>>>
> > >>>> As the project grows both in terms of LOCs and contributors, this
> > >>>> becomes more and more important as it eliminates some potential
> points
> > >>>> of friction without any additional effort.
> > >>>>
> > >>>>  From the discussion, I am leaning towards having a single commit
> with
> > >>>> all the codestyle-related changes. This will avoid sprinkling
> > >>>> refactoring commits all over the place for the next year or more.
> But
> > >>>> if this is the price to pay for having consensus on a tool, then I
> am
> > >>>> ok with gradual implementation. I believe that the value added by
> > >>>> having an automated process of enforcing a codestyle exceeds the
> cost
> > >>>> of the nuisance of gradual refactoring.
> > >>>>
> > >>>> As for the actual format, I like the google-java-format but again,
> if
> > >>>> the community agrees on a different one I would not oppose that (as
> > >>>> long as it does not use the same amount of indentation for method
> args
> > >>>> and method body :P).
> > >>>>
> > >>>> Cheers,
> > >>>> Kostas
> > >>>>
> > >>>> On Wed, Oct 7, 2020 at 10:26 AM Chesnay Schepler <
> [hidden email]>
> > >>>> wrote:
> > >>>>>
> > >>>>> To me, ratchet seems to combine the worst aspects of both
> approaches.
> > >>>>> You get disruptive changes, but only in singular files,
> > >>>>> for something mundane as a typo fix or import change, which would
> be
> > >>>>> annoying to keep separate from the actual functional changes in a
> PR.
> > >>>>>
> > >>>>> On 10/7/2020 10:04 AM, Matthias Pohl wrote:
> > >>>>>> I find the ratchet feature you're suggesting interesting. But
> Arvid
> > >>>> has a
> > >>>>>> point referring to the blog post about ignoring revisions in git
> > >>>>>> blame
> > >>>> [1].
> > >>>>>> Adding the configuration file for commits to ignore revs as
> proposed
> > >>>> in the
> > >>>>>> blog post makes it even easier. One problem I see is that this is
> > >>>>>> not
> > >>>>>> supported by Github (yet?) [2] as mentioned in [1].
> > >>>>>>
> > >>>>>> Considering all that I prefer applying the code style in one go. I
> > >>>> have no
> > >>>>>> strong opinion on what codestyle is the best.
> > >>>>>>
> > >>>>>> PS: We used spotless in the project I previously worked on. It was
> > >>>>>> convenient to use.
> > >>>>>>
> > >>>>>> [1]
> > >>>>>>
> > >>>>
> >
> https://www.moxio.com/blog/43/ignoring-bulk-change-commits-with-git-blame
> > >>>>
> > >>>>>> [2]
> > >>>>>>
> > >>>>
> >
> https://github.community/t/support-ignore-revs-file-in-githubs-blame-view/3256
> > >>>>
> > >>>>>>
> > >>>>>> On Tue, Oct 6, 2020 at 6:00 PM Aljoscha Krettek
> > >>>>>> <[hidden email]>
> > >>>> wrote:
> > >>>>>>
> > >>>>>>> Maybe I wasn't very clear on how the ratchet works. The changes
> are
> > >>>>>>> gradual yes, but it doesn't leave you an option: if you touch a
> > >>>>>>> file
> > >>>> you
> > >>>>>>> will it will have to conform to the coding style afterwards.
> > >>>>>>> It's not
> > >>>>>>> like the previous gradual process we had before where it would be
> > >>>> based
> > >>>>>>> on people actively working towards a style.
> > >>>>>>>
> > >>>>>>> That being said, I also completely like the option of just doing
> > >>>>>>> one
> > >>>> big
> > >>>>>>> change commit.
> > >>>>>>>
> > >>>>>>> Regarding actual coding styles: we're a bit limited by what tools
> > >>>> exist.
> > >>>>>>> I like Spotless because it can be used to both check and apply a
> > >>>> style.
> > >>>>>>> Then you need a formatter that works with Spotless and of those
> we
> > >>>> only
> > >>>>>>> have the Eclipse Formatter, google-java-format, and Prettier.
> > >>>>>>> Prettier
> > >>>>>>> is a Javascript tool that I would like to avoid. Eclipse is
> > >>>>>>> doable but
> > >>>>>>> you need to fiddle with configuration files. I like
> > >>>>>>> google-java-format
> > >>>>>>> because of it's take-it-or-leave-it approach. You either use the
> > >>>>>>> style
> > >>>>>>> or you don't but it's very thorough. The downside is that it
> > >>>>>>> won't do
> > >>>>>>> tabs-only formatting.
> > >>>>>>>
> > >>>>>>> Best,
> > >>>>>>> Aljoscha
> > >>>>>>>
> > >>>>>>> On 06.10.20 17:43, Arvid Heise wrote:
> > >>>>>>>> After having written that I did a quick search, you can even
> > >>>>>>>> use git
> > >>>>>>> blame
> > >>>>>>>> with one big massive change commit [1], which would further
> > >>>>>>>> help the
> > >>>> idea
> > >>>>>>>> of "just get over with it".
> > >>>>>>>>
> > >>>>>>>> With that option, I'd even change all whitespaces if the
> community
> > >>>> thinks
> > >>>>>>>> that it's a better option (a separate discussion that I'll
> gladly
> > >>>> skip).
> > >>>>>>>>
> > >>>>>>>> [1]
> > >>>>>>>>
> > >>>>>>>
> > >>>>
> >
> https://www.moxio.com/blog/43/ignoring-bulk-change-commits-with-git-blame
> > >>>>
> > >>>>>>>> On Tue, Oct 6, 2020 at 5:38 PM Arvid Heise <[hidden email]
> >
> > >>>> wrote:
> > >>>>>>>>
> > >>>>>>>>> I'm also +1 for automatically enforceable code style.
> > >>>>>>>>>
> > >>>>>>>>> I also would just go over it as Chesnay said. While it makes
> some
> > >>>>>>> changes
> > >>>>>>>>> a bit harder to track (inline git blame), it's easy to skip
> > >>>>>>>>> over in
> > >>>> any
> > >>>>>>> git
> > >>>>>>>>> history and if it's only one massive commit, then it's also
> much
> > >>>> easier
> > >>>>>>> to
> > >>>>>>>>> ignore than many gradual changes. Further, if we just do it
> once,
> > >>>> git
> > >>>>>>> blame
> > >>>>>>>>> will quickly become more reliable again.
> > >>>>>>>>>
> > >>>>>>>>> Btw I completely don't care about the code style as long as it
> > >>>>>>>>> plays
> > >>>>>>> well
> > >>>>>>>>> with IntelliJ (it used to be different, but things change :p).
> > >>>>>>>>>
> > >>>>>>>>> On Tue, Oct 6, 2020 at 5:23 PM Chesnay Schepler
> > >>>>>>>>> <[hidden email]
> > >>>>>
> > >>>>>>>>> wrote:
> > >>>>>>>>>
> > >>>>>>>>>> We shouldn't switch to spaces _now_; cutting this bit from
> your
> > >>>>>>> proposal
> > >>>>>>>>>> will massively simplify things and there's hardly any value in
> > >>>> changing
> > >>>>>>>>>> it.
> > >>>>>>>>>>
> > >>>>>>>>>> Also I'm getting rather tired of this constant idea of
> "gradual
> > >>>>>>>>>> application". We've been doing this for 2-3 years now since we
> > >>>>>>>>>> introduced Checkstyle and basically got nowhere. We should
> just
> > >>>> bite
> > >>>>>>> the
> > >>>>>>>>>> bullet and get it over with; we could've solved this whole
> > >>>>>>>>>> problem
> > >>>>>>>>>> already.
> > >>>>>>>>>>
> > >>>>>>>>>> In conclusion, I'm +1 on finally locking down the codestyle
> and
> > >>>>>>> applying
> > >>>>>>>>>> it immediately, I'm -1 on any gradual application scheme
> because
> > >>>> they
> > >>>>>>>>>> _just don't work_.
> > >>>>>>>>>>
> > >>>>>>>>>> On 10/6/2020 2:15 PM, Aljoscha Krettek wrote:
> > >>>>>>>>>>> Hi All,
> > >>>>>>>>>>>
> > >>>>>>>>>>> I know I know, but please keep reading because I recently
> > >>>>>>>>>>> learned
> > >>>>>>>>>>> about some new developments in the area of coding-style
> > >>>> automation.
> > >>>>>>>>>>>
> > >>>>>>>>>>> The tool I would propose we use is Spotless
> > >>>>>>>>>>> (https://github.com/diffplug/spotless). This doesn't come
> > >>>>>>>>>>> with a
> > >>>>>>>>>>> formatter but allows using other popular formatters such as
> > >>>>>>>>>>> google-java-format. The nice thing about Spotless is that it
> > >>>> serves as
> > >>>>>>>>>>> a verifier for CI but can also apply the configured style
> > >>>>>>>>>>> automatically. That is, for the programmer all she has to do
> is
> > >>>> `mvn
> > >>>>>>>>>>> spotless:apply` to fix any style violations.
> > >>>>>>>>>>>
> > >>>>>>>>>>> An interesting feature, which was (somewhat) recently added
> is
> > >>>>>>>>>>> "ratchet"
> > >>>>>>>>>>> (
> > >>>>>>>
> > >>>>
> >
> https://github.com/diffplug/spotless/blob/main/plugin-maven/README.md#ratchet
> > >>>>
> > >>>>>>> ).
> > >>>>>>>>>>> With this, you can set up Spotless to only apply it's rules
> to
> > >>>> files
> > >>>>>>>>>>> that were changed after a configured commit. This would
> allow a
> > >>>>>>>>>>> gradual application of the new coding style instead of one
> big
> > >>>> change.
> > >>>>>>>>>>>
> > >>>>>>>>>>> If we decide to use Spotless, we would of course also have to
> > >>>> decide
> > >>>>>>>>>>> on a coding style. For this I would propose
> google-java-format,
> > >>>> which
> > >>>>>>>>>>> the flink-statefun project uses. The main difference from our
> > >>>> current
> > >>>>>>>>>>> "style" is that this uses spaces instead of tabs for
> > >>>>>>>>>>> indentation.
> > >>>> By
> > >>>>>>>>>>> default it would be 2 spaces but it can be configured to use
> 4
> > >>>> spaces
> > >>>>>>>>>>> which would make code look more or less like our current
> style.
> > >>>> There
> > >>>>>>>>>>> are no more configuration knobs, so using tabs is not an
> > >>>>>>>>>>> option.
> > >>>>>>>>>>>
> > >>>>>>>>>>> Finally, why should we do this? I think most engineers agree
> > >>>>>>>>>>> that
> > >>>>>>>>>>> having a common enforced style is good to have so I only
> > >>>>>>>>>>> want to
> > >>>>>>>>>>> highlight a few thoughts here about things we could improve:
> > >>>>>>>>>>>
> > >>>>>>>>>>>     - No more nits about coding style in reviews, this makes
> it
> > >>>> easier
> > >>>>>>>>>>> for both the reviewer and developer
> > >>>>>>>>>>>
> > >>>>>>>>>>>     - No manual fixing of Checkstyle errors because Spotless
> > >>>>>>>>>>> can
> > >>>> do that
> > >>>>>>>>>>> automatically
> > >>>>>>>>>>>
> > >>>>>>>>>>>     - Because Flink is such a big project little islands of
> > >>>>>>>>>>> coding
> > >>>> style
> > >>>>>>>>>>> have formed between people that commonly work on components.
> It
> > >>>> can be
> > >>>>>>>>>>> a nuisance when you work on a different component and then
> > >>>> reviewers
> > >>>>>>>>>>> don't like your typical coding style. And you first have to
> get
> > >>>> used
> > >>>>>>>>>>> to the slight differences in style when reading code.
> > >>>>>>>>>>>
> > >>>>>>>>>>> There are also downsides I see in this:
> > >>>>>>>>>>>
> > >>>>>>>>>>>     - We break the history, but both "git blame" and modern
> > >>>> IntelliJ can
> > >>>>>>>>>>> ignore whitespace when attributing changes. So for files
> > >>>>>>>>>>> that are
> > >>>>>>>>>>> already "well" formatted not much would change.
> > >>>>>>>>>>>
> > >>>>>>>>>>>     - In the short-term it will be harder to apply changes
> > >>>>>>>>>>> both to
> > >>>>>>> master
> > >>>>>>>>>>> and one of the release-x branches because formatting will be
> > >>>>>>>>>>> different. I think this is not too hard though because
> Spotless
> > >>>> can
> > >>>>>>>>>>> automatically apply the style.
> > >>>>>>>>>>>
> > >>>>>>>>>>> In summary, we would have some short-term pain with this but
> I
> > >>>> think
> > >>>>>>>>>>> it would be good in the long run. What are your thoughts?
> > >>>>>>>>>>>
> > >>>>>>>>>>> Best,
> > >>>>>>>>>>> Aljoscha
> > >>>>>>>>>>>
> > >>>>>>>>>>
> > >>>>>>>>> --
> > >>>>>>>>>
> > >>>>>>>>> Arvid Heise | Senior Java Developer
> > >>>>>>>>>
> > >>>>>>>>> <https://www.ververica.com/>
> > >>>>>>>>>
> > >>>>>>>>> Follow us @VervericaData
> > >>>>>>>>>
> > >>>>>>>>> --
> > >>>>>>>>>
> > >>>>>>>>> Join Flink Forward <https://flink-forward.org/> - The Apache
> > >>>>>>>>> Flink
> > >>>>>>>>> Conference
> > >>>>>>>>>
> > >>>>>>>>> Stream Processing | Event Driven | Real Time
> > >>>>>>>>>
> > >>>>>>>>> --
> > >>>>>>>>>
> > >>>>>>>>> Ververica GmbH | Invalidenstrasse 115, 10115 Berlin, Germany
> > >>>>>>>>>
> > >>>>>>>>> --
> > >>>>>>>>> Ververica GmbH
> > >>>>>>>>> Registered at Amtsgericht Charlottenburg: HRB 158244 B
> > >>>>>>>>> Managing Directors: Timothy Alexander Steinert, Yip Park Tung
> > >>>> Jason, Ji
> > >>>>>>>>> (Toni) Cheng
> > >>>>>>>>>
> > >>>>>>>>
> > >>>>>
> > >>>>
> > >>>
> > >>
> > >
> > >
> >
> >
>

--

Arvid Heise | Senior Java Developer

<https://www.ververica.com/>

Follow us @VervericaData

--

Join Flink Forward <https://flink-forward.org/> - The Apache Flink
Conference

Stream Processing | Event Driven | Real Time

--

Ververica GmbH | Invalidenstrasse 115, 10115 Berlin, Germany

--
Ververica GmbH
Registered at Amtsgericht Charlottenburg: HRB 158244 B
Managing Directors: Timothy Alexander Steinert, Yip Park Tung Jason, Ji
(Toni) Cheng

[Kostas Kloudas-4]

+1 asap from my side as well.

On Wed, Dec 16, 2020 at 8:04 PM Arvid Heise <[hidden email]> wrote:

>
> +1 asap.
>
> On Wed, Dec 16, 2020 at 7:44 PM Robert Metzger <[hidden email]> wrote:
>
> > +1
> >
> > Thanks for driving this.
> >
> > On Wed, Dec 16, 2020 at 7:33 PM Chesnay Schepler <[hidden email]>
> > wrote:
> >
> > > +1 to set this up ASAP. Now's a good time to (finally) finalize this
> > > effort with a new release cycle having started and christmas/vacations
> > > being around the corner.
> > >
> > > On 12/16/2020 7:20 PM, Aljoscha Krettek wrote:
> > > > Let's try and conclude this discussion! I've prepared a PoC that uses
> > > > Spotless with google-java-format to do the formatting:
> > > > https://github.com/aljoscha/flink/commits/flink-xxx-spotless
> > > >
> > > > To summarize from earlier discussion, the main benefits are:
> > > >  - no more worrying about code style, both as reviewer and implementer
> > > >  - everyone can configure their IDE to auto-format, there will never
> > > > be unrelated formatting changes
> > > >
> > > > Also, this uses git's blame.ignoreRevsFile to add a file that tells
> > > > git blame/IntelliJ to ignore the refactoring commit. However, you need
> > > > to manually configure your git for that using:
> > > >
> > > > $ git config blame.ignoreRevsFile .git-blame-ignore-revs
> > > >
> > > > You can check out the PoC, look at the code in an IDE, play around
> > > > with blame/annotations.
> > > >
> > > > By the way, the coding style is not configurable, it’s the Google Java
> > > > Style, wich uses spaces for indentation. In an IDE or on github the
> > > > code looks barely different from the previous formatting at a first
> > > > glance.
> > > >
> > > > For IDE setup, I will add a guide in the README but it boils down to:
> > > >
> > > >  - install the google-java-format plugin, enable it
> > > >  - install the Save Actions plugin, enable "Optimize Imports" and
> > > > "Reformat File"
> > > >
> > > > With this, every time you save, formatting will be applied
> > > > automatically. You will never see formatting complaints from CI
> > > > (except for cases where you break semantical checkstyle rules, such as
> > > > using certain imports that we don't allow.).
> > > >
> > > > What do you think?
> > > >
> > > > Best,
> > > > Aljoscha
> > > >
> > > > On 19.10.20 12:36, Aljoscha Krettek wrote:
> > > >> I don't like checkstyle because it cannot be easily applied from the
> > > >> commandline. I'm happy to learn otherwise, though. And I'd also be
> > > >> very happy about alternative suggestions that can do that.
> > > >>
> > > >> Right now, I think Spotless is the most straightforward tool for
> > > >> that. Also I don't care so much about what style we choose in the
> > > >> end. (If we choose one). My main motivation is that we have a common,
> > > >> strict style that can easily applied via tooling so that we no longer
> > > >> need to comment on coding style in PRs.
> > > >>
> > > >> Aljoscha
> > > >>
> > > >> On 09.10.20 11:11, tison wrote:
> > > >>> +1 on locking on one codestyle and I think that is what current
> > > >>> checkstyle
> > > >>> rules serving.
> > > >>>
> > > >>> For automatic applying part, we suggest developing by IDEA and with
> > > >>> Checkstyle Plugin on IDEA applying checkstyle suggestion is also
> > > >>> automatic.
> > > >>>
> > > >>> One short coming for spotless is that we can hardly adjust rules if
> > the
> > > >>> project has its own issues to overcome. IIRC only several
> > > >>> pre-defined rules
> > > >>> and a clumsy general regex substitution rule can be used.
> > > >>>
> > > >>> FYI my team started with spotless but ended up with checkstyle with
> > few
> > > >>> rules and Checkstyle Plugin for automation.
> > > >>>
> > > >>> Codestyle, in another perspective, is part of cognition of developers
> > > >>> working in project, not something we just apply before pull request.
> > No
> > > >>> matter how much automation introduced, most of developers will
> > converge
> > > >>> working with the configured codestyle.
> > > >>>
> > > >>> Best,
> > > >>> tison.
> > > >>>
> > > >>>
> > > >>> Kostas Kloudas <[hidden email]> 于2020年10月7日周三 下午6:37写道：
> > > >>>
> > > >>>> Hi all,
> > > >>>>
> > > >>>> +1 for enforcing "a" codestyle using "a" tool.
> > > >>>>
> > > >>>> As the project grows both in terms of LOCs and contributors, this
> > > >>>> becomes more and more important as it eliminates some potential
> > points
> > > >>>> of friction without any additional effort.
> > > >>>>
> > > >>>>  From the discussion, I am leaning towards having a single commit
> > with
> > > >>>> all the codestyle-related changes. This will avoid sprinkling
> > > >>>> refactoring commits all over the place for the next year or more.
> > But
> > > >>>> if this is the price to pay for having consensus on a tool, then I
> > am
> > > >>>> ok with gradual implementation. I believe that the value added by
> > > >>>> having an automated process of enforcing a codestyle exceeds the
> > cost
> > > >>>> of the nuisance of gradual refactoring.
> > > >>>>
> > > >>>> As for the actual format, I like the google-java-format but again,
> > if
> > > >>>> the community agrees on a different one I would not oppose that (as
> > > >>>> long as it does not use the same amount of indentation for method
> > args
> > > >>>> and method body :P).
> > > >>>>
> > > >>>> Cheers,
> > > >>>> Kostas
> > > >>>>
> > > >>>> On Wed, Oct 7, 2020 at 10:26 AM Chesnay Schepler <
> > [hidden email]>
> > > >>>> wrote:
> > > >>>>>
> > > >>>>> To me, ratchet seems to combine the worst aspects of both
> > approaches.
> > > >>>>> You get disruptive changes, but only in singular files,
> > > >>>>> for something mundane as a typo fix or import change, which would
> > be
> > > >>>>> annoying to keep separate from the actual functional changes in a
> > PR.
> > > >>>>>
> > > >>>>> On 10/7/2020 10:04 AM, Matthias Pohl wrote:
> > > >>>>>> I find the ratchet feature you're suggesting interesting. But
> > Arvid
> > > >>>> has a
> > > >>>>>> point referring to the blog post about ignoring revisions in git
> > > >>>>>> blame
> > > >>>> [1].
> > > >>>>>> Adding the configuration file for commits to ignore revs as
> > proposed
> > > >>>> in the
> > > >>>>>> blog post makes it even easier. One problem I see is that this is
> > > >>>>>> not
> > > >>>>>> supported by Github (yet?) [2] as mentioned in [1].
> > > >>>>>>
> > > >>>>>> Considering all that I prefer applying the code style in one go. I
> > > >>>> have no
> > > >>>>>> strong opinion on what codestyle is the best.
> > > >>>>>>
> > > >>>>>> PS: We used spotless in the project I previously worked on. It was
> > > >>>>>> convenient to use.
> > > >>>>>>
> > > >>>>>> [1]
> > > >>>>>>
> > > >>>>
> > >
> > https://www.moxio.com/blog/43/ignoring-bulk-change-commits-with-git-blame
> > > >>>>
> > > >>>>>> [2]
> > > >>>>>>
> > > >>>>
> > >
> > https://github.community/t/support-ignore-revs-file-in-githubs-blame-view/3256
> > > >>>>
> > > >>>>>>
> > > >>>>>> On Tue, Oct 6, 2020 at 6:00 PM Aljoscha Krettek
> > > >>>>>> <[hidden email]>
> > > >>>> wrote:
> > > >>>>>>
> > > >>>>>>> Maybe I wasn't very clear on how the ratchet works. The changes
> > are
> > > >>>>>>> gradual yes, but it doesn't leave you an option: if you touch a
> > > >>>>>>> file
> > > >>>> you
> > > >>>>>>> will it will have to conform to the coding style afterwards.
> > > >>>>>>> It's not
> > > >>>>>>> like the previous gradual process we had before where it would be
> > > >>>> based
> > > >>>>>>> on people actively working towards a style.
> > > >>>>>>>
> > > >>>>>>> That being said, I also completely like the option of just doing
> > > >>>>>>> one
> > > >>>> big
> > > >>>>>>> change commit.
> > > >>>>>>>
> > > >>>>>>> Regarding actual coding styles: we're a bit limited by what tools
> > > >>>> exist.
> > > >>>>>>> I like Spotless because it can be used to both check and apply a
> > > >>>> style.
> > > >>>>>>> Then you need a formatter that works with Spotless and of those
> > we
> > > >>>> only
> > > >>>>>>> have the Eclipse Formatter, google-java-format, and Prettier.
> > > >>>>>>> Prettier
> > > >>>>>>> is a Javascript tool that I would like to avoid. Eclipse is
> > > >>>>>>> doable but
> > > >>>>>>> you need to fiddle with configuration files. I like
> > > >>>>>>> google-java-format
> > > >>>>>>> because of it's take-it-or-leave-it approach. You either use the
> > > >>>>>>> style
> > > >>>>>>> or you don't but it's very thorough. The downside is that it
> > > >>>>>>> won't do
> > > >>>>>>> tabs-only formatting.
> > > >>>>>>>
> > > >>>>>>> Best,
> > > >>>>>>> Aljoscha
> > > >>>>>>>
> > > >>>>>>> On 06.10.20 17:43, Arvid Heise wrote:
> > > >>>>>>>> After having written that I did a quick search, you can even
> > > >>>>>>>> use git
> > > >>>>>>> blame
> > > >>>>>>>> with one big massive change commit [1], which would further
> > > >>>>>>>> help the
> > > >>>> idea
> > > >>>>>>>> of "just get over with it".
> > > >>>>>>>>
> > > >>>>>>>> With that option, I'd even change all whitespaces if the
> > community
> > > >>>> thinks
> > > >>>>>>>> that it's a better option (a separate discussion that I'll
> > gladly
> > > >>>> skip).
> > > >>>>>>>>
> > > >>>>>>>> [1]
> > > >>>>>>>>
> > > >>>>>>>
> > > >>>>
> > >
> > https://www.moxio.com/blog/43/ignoring-bulk-change-commits-with-git-blame
> > > >>>>
> > > >>>>>>>> On Tue, Oct 6, 2020 at 5:38 PM Arvid Heise <[hidden email]
> > >
> > > >>>> wrote:
> > > >>>>>>>>
> > > >>>>>>>>> I'm also +1 for automatically enforceable code style.
> > > >>>>>>>>>
> > > >>>>>>>>> I also would just go over it as Chesnay said. While it makes
> > some
> > > >>>>>>> changes
> > > >>>>>>>>> a bit harder to track (inline git blame), it's easy to skip
> > > >>>>>>>>> over in
> > > >>>> any
> > > >>>>>>> git
> > > >>>>>>>>> history and if it's only one massive commit, then it's also
> > much
> > > >>>> easier
> > > >>>>>>> to
> > > >>>>>>>>> ignore than many gradual changes. Further, if we just do it
> > once,
> > > >>>> git
> > > >>>>>>> blame
> > > >>>>>>>>> will quickly become more reliable again.
> > > >>>>>>>>>
> > > >>>>>>>>> Btw I completely don't care about the code style as long as it
> > > >>>>>>>>> plays
> > > >>>>>>> well
> > > >>>>>>>>> with IntelliJ (it used to be different, but things change :p).
> > > >>>>>>>>>
> > > >>>>>>>>> On Tue, Oct 6, 2020 at 5:23 PM Chesnay Schepler
> > > >>>>>>>>> <[hidden email]
> > > >>>>>
> > > >>>>>>>>> wrote:
> > > >>>>>>>>>
> > > >>>>>>>>>> We shouldn't switch to spaces _now_; cutting this bit from
> > your
> > > >>>>>>> proposal
> > > >>>>>>>>>> will massively simplify things and there's hardly any value in
> > > >>>> changing
> > > >>>>>>>>>> it.
> > > >>>>>>>>>>
> > > >>>>>>>>>> Also I'm getting rather tired of this constant idea of
> > "gradual
> > > >>>>>>>>>> application". We've been doing this for 2-3 years now since we
> > > >>>>>>>>>> introduced Checkstyle and basically got nowhere. We should
> > just
> > > >>>> bite
> > > >>>>>>> the
> > > >>>>>>>>>> bullet and get it over with; we could've solved this whole
> > > >>>>>>>>>> problem
> > > >>>>>>>>>> already.
> > > >>>>>>>>>>
> > > >>>>>>>>>> In conclusion, I'm +1 on finally locking down the codestyle
> > and
> > > >>>>>>> applying
> > > >>>>>>>>>> it immediately, I'm -1 on any gradual application scheme
> > because
> > > >>>> they
> > > >>>>>>>>>> _just don't work_.
> > > >>>>>>>>>>
> > > >>>>>>>>>> On 10/6/2020 2:15 PM, Aljoscha Krettek wrote:
> > > >>>>>>>>>>> Hi All,
> > > >>>>>>>>>>>
> > > >>>>>>>>>>> I know I know, but please keep reading because I recently
> > > >>>>>>>>>>> learned
> > > >>>>>>>>>>> about some new developments in the area of coding-style
> > > >>>> automation.
> > > >>>>>>>>>>>
> > > >>>>>>>>>>> The tool I would propose we use is Spotless
> > > >>>>>>>>>>> (https://github.com/diffplug/spotless). This doesn't come
> > > >>>>>>>>>>> with a
> > > >>>>>>>>>>> formatter but allows using other popular formatters such as
> > > >>>>>>>>>>> google-java-format. The nice thing about Spotless is that it
> > > >>>> serves as
> > > >>>>>>>>>>> a verifier for CI but can also apply the configured style
> > > >>>>>>>>>>> automatically. That is, for the programmer all she has to do
> > is
> > > >>>> `mvn
> > > >>>>>>>>>>> spotless:apply` to fix any style violations.
> > > >>>>>>>>>>>
> > > >>>>>>>>>>> An interesting feature, which was (somewhat) recently added
> > is
> > > >>>>>>>>>>> "ratchet"
> > > >>>>>>>>>>> (
> > > >>>>>>>
> > > >>>>
> > >
> > https://github.com/diffplug/spotless/blob/main/plugin-maven/README.md#ratchet
> > > >>>>
> > > >>>>>>> ).
> > > >>>>>>>>>>> With this, you can set up Spotless to only apply it's rules
> > to
> > > >>>> files
> > > >>>>>>>>>>> that were changed after a configured commit. This would
> > allow a
> > > >>>>>>>>>>> gradual application of the new coding style instead of one
> > big
> > > >>>> change.
> > > >>>>>>>>>>>
> > > >>>>>>>>>>> If we decide to use Spotless, we would of course also have to
> > > >>>> decide
> > > >>>>>>>>>>> on a coding style. For this I would propose
> > google-java-format,
> > > >>>> which
> > > >>>>>>>>>>> the flink-statefun project uses. The main difference from our
> > > >>>> current
> > > >>>>>>>>>>> "style" is that this uses spaces instead of tabs for
> > > >>>>>>>>>>> indentation.
> > > >>>> By
> > > >>>>>>>>>>> default it would be 2 spaces but it can be configured to use
> > 4
> > > >>>> spaces
> > > >>>>>>>>>>> which would make code look more or less like our current
> > style.
> > > >>>> There
> > > >>>>>>>>>>> are no more configuration knobs, so using tabs is not an
> > > >>>>>>>>>>> option.
> > > >>>>>>>>>>>
> > > >>>>>>>>>>> Finally, why should we do this? I think most engineers agree
> > > >>>>>>>>>>> that
> > > >>>>>>>>>>> having a common enforced style is good to have so I only
> > > >>>>>>>>>>> want to
> > > >>>>>>>>>>> highlight a few thoughts here about things we could improve:
> > > >>>>>>>>>>>
> > > >>>>>>>>>>>     - No more nits about coding style in reviews, this makes
> > it
> > > >>>> easier
> > > >>>>>>>>>>> for both the reviewer and developer
> > > >>>>>>>>>>>
> > > >>>>>>>>>>>     - No manual fixing of Checkstyle errors because Spotless
> > > >>>>>>>>>>> can
> > > >>>> do that
> > > >>>>>>>>>>> automatically
> > > >>>>>>>>>>>
> > > >>>>>>>>>>>     - Because Flink is such a big project little islands of
> > > >>>>>>>>>>> coding
> > > >>>> style
> > > >>>>>>>>>>> have formed between people that commonly work on components.
> > It
> > > >>>> can be
> > > >>>>>>>>>>> a nuisance when you work on a different component and then
> > > >>>> reviewers
> > > >>>>>>>>>>> don't like your typical coding style. And you first have to
> > get
> > > >>>> used
> > > >>>>>>>>>>> to the slight differences in style when reading code.
> > > >>>>>>>>>>>
> > > >>>>>>>>>>> There are also downsides I see in this:
> > > >>>>>>>>>>>
> > > >>>>>>>>>>>     - We break the history, but both "git blame" and modern
> > > >>>> IntelliJ can
> > > >>>>>>>>>>> ignore whitespace when attributing changes. So for files
> > > >>>>>>>>>>> that are
> > > >>>>>>>>>>> already "well" formatted not much would change.
> > > >>>>>>>>>>>
> > > >>>>>>>>>>>     - In the short-term it will be harder to apply changes
> > > >>>>>>>>>>> both to
> > > >>>>>>> master
> > > >>>>>>>>>>> and one of the release-x branches because formatting will be
> > > >>>>>>>>>>> different. I think this is not too hard though because
> > Spotless
> > > >>>> can
> > > >>>>>>>>>>> automatically apply the style.
> > > >>>>>>>>>>>
> > > >>>>>>>>>>> In summary, we would have some short-term pain with this but
> > I
> > > >>>> think
> > > >>>>>>>>>>> it would be good in the long run. What are your thoughts?
> > > >>>>>>>>>>>
> > > >>>>>>>>>>> Best,
> > > >>>>>>>>>>> Aljoscha
> > > >>>>>>>>>>>
> > > >>>>>>>>>>
> > > >>>>>>>>> --
> > > >>>>>>>>>
> > > >>>>>>>>> Arvid Heise | Senior Java Developer
> > > >>>>>>>>>
> > > >>>>>>>>> <https://www.ververica.com/>
> > > >>>>>>>>>
> > > >>>>>>>>> Follow us @VervericaData
> > > >>>>>>>>>
> > > >>>>>>>>> --
> > > >>>>>>>>>
> > > >>>>>>>>> Join Flink Forward <https://flink-forward.org/> - The Apache
> > > >>>>>>>>> Flink
> > > >>>>>>>>> Conference
> > > >>>>>>>>>
> > > >>>>>>>>> Stream Processing | Event Driven | Real Time
> > > >>>>>>>>>
> > > >>>>>>>>> --
> > > >>>>>>>>>
> > > >>>>>>>>> Ververica GmbH | Invalidenstrasse 115, 10115 Berlin, Germany
> > > >>>>>>>>>
> > > >>>>>>>>> --
> > > >>>>>>>>> Ververica GmbH
> > > >>>>>>>>> Registered at Amtsgericht Charlottenburg: HRB 158244 B
> > > >>>>>>>>> Managing Directors: Timothy Alexander Steinert, Yip Park Tung
> > > >>>> Jason, Ji
> > > >>>>>>>>> (Toni) Cheng
> > > >>>>>>>>>
> > > >>>>>>>>
> > > >>>>>
> > > >>>>
> > > >>>
> > > >>
> > > >
> > > >
> > >
> > >
> >
>
>
> --
>
> Arvid Heise | Senior Java Developer
>
> <https://www.ververica.com/>
>
> Follow us @VervericaData
>
> --
>
> Join Flink Forward <https://flink-forward.org/> - The Apache Flink
> Conference
>
> Stream Processing | Event Driven | Real Time
>
> --
>
> Ververica GmbH | Invalidenstrasse 115, 10115 Berlin, Germany
>
> --
> Ververica GmbH
> Registered at Amtsgericht Charlottenburg: HRB 158244 B
> Managing Directors: Timothy Alexander Steinert, Yip Park Tung Jason, Ji
> (Toni) Cheng

[Xingbo Huang]

+1 asap. Spotless can greatly help us save the time of fixing checkstyle
errors.

Best,
Xingbo

Kostas Kloudas <[hidden email]> 于2020年12月17日周四 上午4:14写道：

> +1 asap from my side as well.
>
> On Wed, Dec 16, 2020 at 8:04 PM Arvid Heise <[hidden email]> wrote:
> >
> > +1 asap.
> >
> > On Wed, Dec 16, 2020 at 7:44 PM Robert Metzger <[hidden email]>
> wrote:
> >
> > > +1
> > >
> > > Thanks for driving this.
> > >
> > > On Wed, Dec 16, 2020 at 7:33 PM Chesnay Schepler <[hidden email]>
> > > wrote:
> > >
> > > > +1 to set this up ASAP. Now's a good time to (finally) finalize this
> > > > effort with a new release cycle having started and
> christmas/vacations
> > > > being around the corner.
> > > >
> > > > On 12/16/2020 7:20 PM, Aljoscha Krettek wrote:
> > > > > Let's try and conclude this discussion! I've prepared a PoC that
> uses
> > > > > Spotless with google-java-format to do the formatting:
> > > > > https://github.com/aljoscha/flink/commits/flink-xxx-spotless
> > > > >
> > > > > To summarize from earlier discussion, the main benefits are:
> > > > >  - no more worrying about code style, both as reviewer and
> implementer
> > > > >  - everyone can configure their IDE to auto-format, there will
> never
> > > > > be unrelated formatting changes
> > > > >
> > > > > Also, this uses git's blame.ignoreRevsFile to add a file that tells
> > > > > git blame/IntelliJ to ignore the refactoring commit. However, you
> need
> > > > > to manually configure your git for that using:
> > > > >
> > > > > $ git config blame.ignoreRevsFile .git-blame-ignore-revs
> > > > >
> > > > > You can check out the PoC, look at the code in an IDE, play around
> > > > > with blame/annotations.
> > > > >
> > > > > By the way, the coding style is not configurable, it’s the Google
> Java
> > > > > Style, wich uses spaces for indentation. In an IDE or on github the
> > > > > code looks barely different from the previous formatting at a first
> > > > > glance.
> > > > >
> > > > > For IDE setup, I will add a guide in the README but it boils down
> to:
> > > > >
> > > > >  - install the google-java-format plugin, enable it
> > > > >  - install the Save Actions plugin, enable "Optimize Imports" and
> > > > > "Reformat File"
> > > > >
> > > > > With this, every time you save, formatting will be applied
> > > > > automatically. You will never see formatting complaints from CI
> > > > > (except for cases where you break semantical checkstyle rules,
> such as
> > > > > using certain imports that we don't allow.).
> > > > >
> > > > > What do you think?
> > > > >
> > > > > Best,
> > > > > Aljoscha
> > > > >
> > > > > On 19.10.20 12:36, Aljoscha Krettek wrote:
> > > > >> I don't like checkstyle because it cannot be easily applied from
> the
> > > > >> commandline. I'm happy to learn otherwise, though. And I'd also be
> > > > >> very happy about alternative suggestions that can do that.
> > > > >>
> > > > >> Right now, I think Spotless is the most straightforward tool for
> > > > >> that. Also I don't care so much about what style we choose in the
> > > > >> end. (If we choose one). My main motivation is that we have a
> common,
> > > > >> strict style that can easily applied via tooling so that we no
> longer
> > > > >> need to comment on coding style in PRs.
> > > > >>
> > > > >> Aljoscha
> > > > >>
> > > > >> On 09.10.20 11:11, tison wrote:
> > > > >>> +1 on locking on one codestyle and I think that is what current
> > > > >>> checkstyle
> > > > >>> rules serving.
> > > > >>>
> > > > >>> For automatic applying part, we suggest developing by IDEA and
> with
> > > > >>> Checkstyle Plugin on IDEA applying checkstyle suggestion is also
> > > > >>> automatic.
> > > > >>>
> > > > >>> One short coming for spotless is that we can hardly adjust rules
> if
> > > the
> > > > >>> project has its own issues to overcome. IIRC only several
> > > > >>> pre-defined rules
> > > > >>> and a clumsy general regex substitution rule can be used.
> > > > >>>
> > > > >>> FYI my team started with spotless but ended up with checkstyle
> with
> > > few
> > > > >>> rules and Checkstyle Plugin for automation.
> > > > >>>
> > > > >>> Codestyle, in another perspective, is part of cognition of
> developers
> > > > >>> working in project, not something we just apply before pull
> request.
> > > No
> > > > >>> matter how much automation introduced, most of developers will
> > > converge
> > > > >>> working with the configured codestyle.
> > > > >>>
> > > > >>> Best,
> > > > >>> tison.
> > > > >>>
> > > > >>>
> > > > >>> Kostas Kloudas <[hidden email]> 于2020年10月7日周三 下午6:37写道：
> > > > >>>
> > > > >>>> Hi all,
> > > > >>>>
> > > > >>>> +1 for enforcing "a" codestyle using "a" tool.
> > > > >>>>
> > > > >>>> As the project grows both in terms of LOCs and contributors,
> this
> > > > >>>> becomes more and more important as it eliminates some potential
> > > points
> > > > >>>> of friction without any additional effort.
> > > > >>>>
> > > > >>>>  From the discussion, I am leaning towards having a single
> commit
> > > with
> > > > >>>> all the codestyle-related changes. This will avoid sprinkling
> > > > >>>> refactoring commits all over the place for the next year or
> more.
> > > But
> > > > >>>> if this is the price to pay for having consensus on a tool,
> then I
> > > am
> > > > >>>> ok with gradual implementation. I believe that the value added
> by
> > > > >>>> having an automated process of enforcing a codestyle exceeds the
> > > cost
> > > > >>>> of the nuisance of gradual refactoring.
> > > > >>>>
> > > > >>>> As for the actual format, I like the google-java-format but
> again,
> > > if
> > > > >>>> the community agrees on a different one I would not oppose that
> (as
> > > > >>>> long as it does not use the same amount of indentation for
> method
> > > args
> > > > >>>> and method body :P).
> > > > >>>>
> > > > >>>> Cheers,
> > > > >>>> Kostas
> > > > >>>>
> > > > >>>> On Wed, Oct 7, 2020 at 10:26 AM Chesnay Schepler <
> > > [hidden email]>
> > > > >>>> wrote:
> > > > >>>>>
> > > > >>>>> To me, ratchet seems to combine the worst aspects of both
> > > approaches.
> > > > >>>>> You get disruptive changes, but only in singular files,
> > > > >>>>> for something mundane as a typo fix or import change, which
> would
> > > be
> > > > >>>>> annoying to keep separate from the actual functional changes
> in a
> > > PR.
> > > > >>>>>
> > > > >>>>> On 10/7/2020 10:04 AM, Matthias Pohl wrote:
> > > > >>>>>> I find the ratchet feature you're suggesting interesting. But
> > > Arvid
> > > > >>>> has a
> > > > >>>>>> point referring to the blog post about ignoring revisions in
> git
> > > > >>>>>> blame
> > > > >>>> [1].
> > > > >>>>>> Adding the configuration file for commits to ignore revs as
> > > proposed
> > > > >>>> in the
> > > > >>>>>> blog post makes it even easier. One problem I see is that
> this is
> > > > >>>>>> not
> > > > >>>>>> supported by Github (yet?) [2] as mentioned in [1].
> > > > >>>>>>
> > > > >>>>>> Considering all that I prefer applying the code style in one
> go. I
> > > > >>>> have no
> > > > >>>>>> strong opinion on what codestyle is the best.
> > > > >>>>>>
> > > > >>>>>> PS: We used spotless in the project I previously worked on.
> It was
> > > > >>>>>> convenient to use.
> > > > >>>>>>
> > > > >>>>>> [1]
> > > > >>>>>>
> > > > >>>>
> > > >
> > >
> https://www.moxio.com/blog/43/ignoring-bulk-change-commits-with-git-blame
> > > > >>>>
> > > > >>>>>> [2]
> > > > >>>>>>
> > > > >>>>
> > > >
> > >
> https://github.community/t/support-ignore-revs-file-in-githubs-blame-view/3256
> > > > >>>>
> > > > >>>>>>
> > > > >>>>>> On Tue, Oct 6, 2020 at 6:00 PM Aljoscha Krettek
> > > > >>>>>> <[hidden email]>
> > > > >>>> wrote:
> > > > >>>>>>
> > > > >>>>>>> Maybe I wasn't very clear on how the ratchet works. The
> changes
> > > are
> > > > >>>>>>> gradual yes, but it doesn't leave you an option: if you
> touch a
> > > > >>>>>>> file
> > > > >>>> you
> > > > >>>>>>> will it will have to conform to the coding style afterwards.
> > > > >>>>>>> It's not
> > > > >>>>>>> like the previous gradual process we had before where it
> would be
> > > > >>>> based
> > > > >>>>>>> on people actively working towards a style.
> > > > >>>>>>>
> > > > >>>>>>> That being said, I also completely like the option of just
> doing
> > > > >>>>>>> one
> > > > >>>> big
> > > > >>>>>>> change commit.
> > > > >>>>>>>
> > > > >>>>>>> Regarding actual coding styles: we're a bit limited by what
> tools
> > > > >>>> exist.
> > > > >>>>>>> I like Spotless because it can be used to both check and
> apply a
> > > > >>>> style.
> > > > >>>>>>> Then you need a formatter that works with Spotless and of
> those
> > > we
> > > > >>>> only
> > > > >>>>>>> have the Eclipse Formatter, google-java-format, and Prettier.
> > > > >>>>>>> Prettier
> > > > >>>>>>> is a Javascript tool that I would like to avoid. Eclipse is
> > > > >>>>>>> doable but
> > > > >>>>>>> you need to fiddle with configuration files. I like
> > > > >>>>>>> google-java-format
> > > > >>>>>>> because of it's take-it-or-leave-it approach. You either use
> the
> > > > >>>>>>> style
> > > > >>>>>>> or you don't but it's very thorough. The downside is that it
> > > > >>>>>>> won't do
> > > > >>>>>>> tabs-only formatting.
> > > > >>>>>>>
> > > > >>>>>>> Best,
> > > > >>>>>>> Aljoscha
> > > > >>>>>>>
> > > > >>>>>>> On 06.10.20 17:43, Arvid Heise wrote:
> > > > >>>>>>>> After having written that I did a quick search, you can even
> > > > >>>>>>>> use git
> > > > >>>>>>> blame
> > > > >>>>>>>> with one big massive change commit [1], which would further
> > > > >>>>>>>> help the
> > > > >>>> idea
> > > > >>>>>>>> of "just get over with it".
> > > > >>>>>>>>
> > > > >>>>>>>> With that option, I'd even change all whitespaces if the
> > > community
> > > > >>>> thinks
> > > > >>>>>>>> that it's a better option (a separate discussion that I'll
> > > gladly
> > > > >>>> skip).
> > > > >>>>>>>>
> > > > >>>>>>>> [1]
> > > > >>>>>>>>
> > > > >>>>>>>
> > > > >>>>
> > > >
> > >
> https://www.moxio.com/blog/43/ignoring-bulk-change-commits-with-git-blame
> > > > >>>>
> > > > >>>>>>>> On Tue, Oct 6, 2020 at 5:38 PM Arvid Heise <
> [hidden email]
> > > >
> > > > >>>> wrote:
> > > > >>>>>>>>
> > > > >>>>>>>>> I'm also +1 for automatically enforceable code style.
> > > > >>>>>>>>>
> > > > >>>>>>>>> I also would just go over it as Chesnay said. While it
> makes
> > > some
> > > > >>>>>>> changes
> > > > >>>>>>>>> a bit harder to track (inline git blame), it's easy to skip
> > > > >>>>>>>>> over in
> > > > >>>> any
> > > > >>>>>>> git
> > > > >>>>>>>>> history and if it's only one massive commit, then it's also
> > > much
> > > > >>>> easier
> > > > >>>>>>> to
> > > > >>>>>>>>> ignore than many gradual changes. Further, if we just do it
> > > once,
> > > > >>>> git
> > > > >>>>>>> blame
> > > > >>>>>>>>> will quickly become more reliable again.
> > > > >>>>>>>>>
> > > > >>>>>>>>> Btw I completely don't care about the code style as long
> as it
> > > > >>>>>>>>> plays
> > > > >>>>>>> well
> > > > >>>>>>>>> with IntelliJ (it used to be different, but things change
> :p).
> > > > >>>>>>>>>
> > > > >>>>>>>>> On Tue, Oct 6, 2020 at 5:23 PM Chesnay Schepler
> > > > >>>>>>>>> <[hidden email]
> > > > >>>>>
> > > > >>>>>>>>> wrote:
> > > > >>>>>>>>>
> > > > >>>>>>>>>> We shouldn't switch to spaces _now_; cutting this bit from
> > > your
> > > > >>>>>>> proposal
> > > > >>>>>>>>>> will massively simplify things and there's hardly any
> value in
> > > > >>>> changing
> > > > >>>>>>>>>> it.
> > > > >>>>>>>>>>
> > > > >>>>>>>>>> Also I'm getting rather tired of this constant idea of
> > > "gradual
> > > > >>>>>>>>>> application". We've been doing this for 2-3 years now
> since we
> > > > >>>>>>>>>> introduced Checkstyle and basically got nowhere. We should
> > > just
> > > > >>>> bite
> > > > >>>>>>> the
> > > > >>>>>>>>>> bullet and get it over with; we could've solved this whole
> > > > >>>>>>>>>> problem
> > > > >>>>>>>>>> already.
> > > > >>>>>>>>>>
> > > > >>>>>>>>>> In conclusion, I'm +1 on finally locking down the
> codestyle
> > > and
> > > > >>>>>>> applying
> > > > >>>>>>>>>> it immediately, I'm -1 on any gradual application scheme
> > > because
> > > > >>>> they
> > > > >>>>>>>>>> _just don't work_.
> > > > >>>>>>>>>>
> > > > >>>>>>>>>> On 10/6/2020 2:15 PM, Aljoscha Krettek wrote:
> > > > >>>>>>>>>>> Hi All,
> > > > >>>>>>>>>>>
> > > > >>>>>>>>>>> I know I know, but please keep reading because I recently
> > > > >>>>>>>>>>> learned
> > > > >>>>>>>>>>> about some new developments in the area of coding-style
> > > > >>>> automation.
> > > > >>>>>>>>>>>
> > > > >>>>>>>>>>> The tool I would propose we use is Spotless
> > > > >>>>>>>>>>> (https://github.com/diffplug/spotless). This doesn't
> come
> > > > >>>>>>>>>>> with a
> > > > >>>>>>>>>>> formatter but allows using other popular formatters such
> as
> > > > >>>>>>>>>>> google-java-format. The nice thing about Spotless is
> that it
> > > > >>>> serves as
> > > > >>>>>>>>>>> a verifier for CI but can also apply the configured style
> > > > >>>>>>>>>>> automatically. That is, for the programmer all she has
> to do
> > > is
> > > > >>>> `mvn
> > > > >>>>>>>>>>> spotless:apply` to fix any style violations.
> > > > >>>>>>>>>>>
> > > > >>>>>>>>>>> An interesting feature, which was (somewhat) recently
> added
> > > is
> > > > >>>>>>>>>>> "ratchet"
> > > > >>>>>>>>>>> (
> > > > >>>>>>>
> > > > >>>>
> > > >
> > >
> https://github.com/diffplug/spotless/blob/main/plugin-maven/README.md#ratchet
> > > > >>>>
> > > > >>>>>>> ).
> > > > >>>>>>>>>>> With this, you can set up Spotless to only apply it's
> rules
> > > to
> > > > >>>> files
> > > > >>>>>>>>>>> that were changed after a configured commit. This would
> > > allow a
> > > > >>>>>>>>>>> gradual application of the new coding style instead of
> one
> > > big
> > > > >>>> change.
> > > > >>>>>>>>>>>
> > > > >>>>>>>>>>> If we decide to use Spotless, we would of course also
> have to
> > > > >>>> decide
> > > > >>>>>>>>>>> on a coding style. For this I would propose
> > > google-java-format,
> > > > >>>> which
> > > > >>>>>>>>>>> the flink-statefun project uses. The main difference
> from our
> > > > >>>> current
> > > > >>>>>>>>>>> "style" is that this uses spaces instead of tabs for
> > > > >>>>>>>>>>> indentation.
> > > > >>>> By
> > > > >>>>>>>>>>> default it would be 2 spaces but it can be configured to
> use
> > > 4
> > > > >>>> spaces
> > > > >>>>>>>>>>> which would make code look more or less like our current
> > > style.
> > > > >>>> There
> > > > >>>>>>>>>>> are no more configuration knobs, so using tabs is not an
> > > > >>>>>>>>>>> option.
> > > > >>>>>>>>>>>
> > > > >>>>>>>>>>> Finally, why should we do this? I think most engineers
> agree
> > > > >>>>>>>>>>> that
> > > > >>>>>>>>>>> having a common enforced style is good to have so I only
> > > > >>>>>>>>>>> want to
> > > > >>>>>>>>>>> highlight a few thoughts here about things we could
> improve:
> > > > >>>>>>>>>>>
> > > > >>>>>>>>>>>     - No more nits about coding style in reviews, this
> makes
> > > it
> > > > >>>> easier
> > > > >>>>>>>>>>> for both the reviewer and developer
> > > > >>>>>>>>>>>
> > > > >>>>>>>>>>>     - No manual fixing of Checkstyle errors because
> Spotless
> > > > >>>>>>>>>>> can
> > > > >>>> do that
> > > > >>>>>>>>>>> automatically
> > > > >>>>>>>>>>>
> > > > >>>>>>>>>>>     - Because Flink is such a big project little islands
> of
> > > > >>>>>>>>>>> coding
> > > > >>>> style
> > > > >>>>>>>>>>> have formed between people that commonly work on
> components.
> > > It
> > > > >>>> can be
> > > > >>>>>>>>>>> a nuisance when you work on a different component and
> then
> > > > >>>> reviewers
> > > > >>>>>>>>>>> don't like your typical coding style. And you first have
> to
> > > get
> > > > >>>> used
> > > > >>>>>>>>>>> to the slight differences in style when reading code.
> > > > >>>>>>>>>>>
> > > > >>>>>>>>>>> There are also downsides I see in this:
> > > > >>>>>>>>>>>
> > > > >>>>>>>>>>>     - We break the history, but both "git blame" and
> modern
> > > > >>>> IntelliJ can
> > > > >>>>>>>>>>> ignore whitespace when attributing changes. So for files
> > > > >>>>>>>>>>> that are
> > > > >>>>>>>>>>> already "well" formatted not much would change.
> > > > >>>>>>>>>>>
> > > > >>>>>>>>>>>     - In the short-term it will be harder to apply
> changes
> > > > >>>>>>>>>>> both to
> > > > >>>>>>> master
> > > > >>>>>>>>>>> and one of the release-x branches because formatting
> will be
> > > > >>>>>>>>>>> different. I think this is not too hard though because
> > > Spotless
> > > > >>>> can
> > > > >>>>>>>>>>> automatically apply the style.
> > > > >>>>>>>>>>>
> > > > >>>>>>>>>>> In summary, we would have some short-term pain with this
> but
> > > I
> > > > >>>> think
> > > > >>>>>>>>>>> it would be good in the long run. What are your thoughts?
> > > > >>>>>>>>>>>
> > > > >>>>>>>>>>> Best,
> > > > >>>>>>>>>>> Aljoscha
> > > > >>>>>>>>>>>
> > > > >>>>>>>>>>
> > > > >>>>>>>>> --
> > > > >>>>>>>>>
> > > > >>>>>>>>> Arvid Heise | Senior Java Developer
> > > > >>>>>>>>>
> > > > >>>>>>>>> <https://www.ververica.com/>
> > > > >>>>>>>>>
> > > > >>>>>>>>> Follow us @VervericaData
> > > > >>>>>>>>>
> > > > >>>>>>>>> --
> > > > >>>>>>>>>
> > > > >>>>>>>>> Join Flink Forward <https://flink-forward.org/> - The
> Apache
> > > > >>>>>>>>> Flink
> > > > >>>>>>>>> Conference
> > > > >>>>>>>>>
> > > > >>>>>>>>> Stream Processing | Event Driven | Real Time
> > > > >>>>>>>>>
> > > > >>>>>>>>> --
> > > > >>>>>>>>>
> > > > >>>>>>>>> Ververica GmbH | Invalidenstrasse 115, 10115 Berlin,
> Germany
> > > > >>>>>>>>>
> > > > >>>>>>>>> --
> > > > >>>>>>>>> Ververica GmbH
> > > > >>>>>>>>> Registered at Amtsgericht Charlottenburg: HRB 158244 B
> > > > >>>>>>>>> Managing Directors: Timothy Alexander Steinert, Yip Park
> Tung
> > > > >>>> Jason, Ji
> > > > >>>>>>>>> (Toni) Cheng
> > > > >>>>>>>>>
> > > > >>>>>>>>
> > > > >>>>>
> > > > >>>>
> > > > >>>
> > > > >>
> > > > >
> > > > >
> > > >
> > > >
> > >
> >
> >
> > --
> >
> > Arvid Heise | Senior Java Developer
> >
> > <https://www.ververica.com/>
> >
> > Follow us @VervericaData
> >
> > --
> >
> > Join Flink Forward <https://flink-forward.org/> - The Apache Flink
> > Conference
> >
> > Stream Processing | Event Driven | Real Time
> >
> > --
> >
> > Ververica GmbH | Invalidenstrasse 115, 10115 Berlin, Germany
> >
> > --
> > Ververica GmbH
> > Registered at Amtsgericht Charlottenburg: HRB 158244 B
> > Managing Directors: Timothy Alexander Steinert, Yip Park Tung Jason, Ji
> > (Toni) Cheng
>

[Wei Zhong-2]

+1 for the coding style automation. Thanks for driving this!

Best,
Wei

> 在 2020年12月17日，10:10，Xingbo Huang <[hidden email]> 写道：
>
> +1 asap. Spotless can greatly help us save the time of fixing checkstyle
> errors.
>
> Best,
> Xingbo
>
> Kostas Kloudas <[hidden email]> 于2020年12月17日周四 上午4:14写道：
>
>> +1 asap from my side as well.
>>
>> On Wed, Dec 16, 2020 at 8:04 PM Arvid Heise <[hidden email]> wrote:
>>>
>>> +1 asap.
>>>
>>> On Wed, Dec 16, 2020 at 7:44 PM Robert Metzger <[hidden email]>
>> wrote:
>>>
>>>> +1
>>>>
>>>> Thanks for driving this.
>>>>
>>>> On Wed, Dec 16, 2020 at 7:33 PM Chesnay Schepler <[hidden email]>
>>>> wrote:
>>>>
>>>>> +1 to set this up ASAP. Now's a good time to (finally) finalize this
>>>>> effort with a new release cycle having started and
>> christmas/vacations
>>>>> being around the corner.
>>>>>
>>>>> On 12/16/2020 7:20 PM, Aljoscha Krettek wrote:
>>>>>> Let's try and conclude this discussion! I've prepared a PoC that
>> uses
>>>>>> Spotless with google-java-format to do the formatting:
>>>>>> https://github.com/aljoscha/flink/commits/flink-xxx-spotless
>>>>>>
>>>>>> To summarize from earlier discussion, the main benefits are:
>>>>>> - no more worrying about code style, both as reviewer and
>> implementer
>>>>>> - everyone can configure their IDE to auto-format, there will
>> never
>>>>>> be unrelated formatting changes
>>>>>>
>>>>>> Also, this uses git's blame.ignoreRevsFile to add a file that tells
>>>>>> git blame/IntelliJ to ignore the refactoring commit. However, you
>> need
>>>>>> to manually configure your git for that using:
>>>>>>
>>>>>> $ git config blame.ignoreRevsFile .git-blame-ignore-revs
>>>>>>
>>>>>> You can check out the PoC, look at the code in an IDE, play around
>>>>>> with blame/annotations.
>>>>>>
>>>>>> By the way, the coding style is not configurable, it’s the Google
>> Java
>>>>>> Style, wich uses spaces for indentation. In an IDE or on github the
>>>>>> code looks barely different from the previous formatting at a first
>>>>>> glance.
>>>>>>
>>>>>> For IDE setup, I will add a guide in the README but it boils down
>> to:
>>>>>>
>>>>>> - install the google-java-format plugin, enable it
>>>>>> - install the Save Actions plugin, enable "Optimize Imports" and
>>>>>> "Reformat File"
>>>>>>
>>>>>> With this, every time you save, formatting will be applied
>>>>>> automatically. You will never see formatting complaints from CI
>>>>>> (except for cases where you break semantical checkstyle rules,
>> such as
>>>>>> using certain imports that we don't allow.).
>>>>>>
>>>>>> What do you think?
>>>>>>
>>>>>> Best,
>>>>>> Aljoscha
>>>>>>
>>>>>> On 19.10.20 12:36, Aljoscha Krettek wrote:
>>>>>>> I don't like checkstyle because it cannot be easily applied from
>> the
>>>>>>> commandline. I'm happy to learn otherwise, though. And I'd also be
>>>>>>> very happy about alternative suggestions that can do that.
>>>>>>>
>>>>>>> Right now, I think Spotless is the most straightforward tool for
>>>>>>> that. Also I don't care so much about what style we choose in the
>>>>>>> end. (If we choose one). My main motivation is that we have a
>> common,
>>>>>>> strict style that can easily applied via tooling so that we no
>> longer
>>>>>>> need to comment on coding style in PRs.
>>>>>>>
>>>>>>> Aljoscha
>>>>>>>
>>>>>>> On 09.10.20 11:11, tison wrote:
>>>>>>>> +1 on locking on one codestyle and I think that is what current
>>>>>>>> checkstyle
>>>>>>>> rules serving.
>>>>>>>>
>>>>>>>> For automatic applying part, we suggest developing by IDEA and
>> with
>>>>>>>> Checkstyle Plugin on IDEA applying checkstyle suggestion is also
>>>>>>>> automatic.
>>>>>>>>
>>>>>>>> One short coming for spotless is that we can hardly adjust rules
>> if
>>>> the
>>>>>>>> project has its own issues to overcome. IIRC only several
>>>>>>>> pre-defined rules
>>>>>>>> and a clumsy general regex substitution rule can be used.
>>>>>>>>
>>>>>>>> FYI my team started with spotless but ended up with checkstyle
>> with
>>>> few
>>>>>>>> rules and Checkstyle Plugin for automation.
>>>>>>>>
>>>>>>>> Codestyle, in another perspective, is part of cognition of
>> developers
>>>>>>>> working in project, not something we just apply before pull
>> request.
>>>> No
>>>>>>>> matter how much automation introduced, most of developers will
>>>> converge
>>>>>>>> working with the configured codestyle.
>>>>>>>>
>>>>>>>> Best,
>>>>>>>> tison.
>>>>>>>>
>>>>>>>>
>>>>>>>> Kostas Kloudas <[hidden email]> 于2020年10月7日周三 下午6:37写道：
>>>>>>>>
>>>>>>>>> Hi all,
>>>>>>>>>
>>>>>>>>> +1 for enforcing "a" codestyle using "a" tool.
>>>>>>>>>
>>>>>>>>> As the project grows both in terms of LOCs and contributors,
>> this
>>>>>>>>> becomes more and more important as it eliminates some potential
>>>> points
>>>>>>>>> of friction without any additional effort.
>>>>>>>>>
>>>>>>>>> From the discussion, I am leaning towards having a single
>> commit
>>>> with
>>>>>>>>> all the codestyle-related changes. This will avoid sprinkling
>>>>>>>>> refactoring commits all over the place for the next year or
>> more.
>>>> But
>>>>>>>>> if this is the price to pay for having consensus on a tool,
>> then I
>>>> am
>>>>>>>>> ok with gradual implementation. I believe that the value added
>> by
>>>>>>>>> having an automated process of enforcing a codestyle exceeds the
>>>> cost
>>>>>>>>> of the nuisance of gradual refactoring.
>>>>>>>>>
>>>>>>>>> As for the actual format, I like the google-java-format but
>> again,
>>>> if
>>>>>>>>> the community agrees on a different one I would not oppose that
>> (as
>>>>>>>>> long as it does not use the same amount of indentation for
>> method
>>>> args
>>>>>>>>> and method body :P).
>>>>>>>>>
>>>>>>>>> Cheers,
>>>>>>>>> Kostas
>>>>>>>>>
>>>>>>>>> On Wed, Oct 7, 2020 at 10:26 AM Chesnay Schepler <
>>>> [hidden email]>
>>>>>>>>> wrote:
>>>>>>>>>>
>>>>>>>>>> To me, ratchet seems to combine the worst aspects of both
>>>> approaches.
>>>>>>>>>> You get disruptive changes, but only in singular files,
>>>>>>>>>> for something mundane as a typo fix or import change, which
>> would
>>>> be
>>>>>>>>>> annoying to keep separate from the actual functional changes
>> in a
>>>> PR.
>>>>>>>>>>
>>>>>>>>>> On 10/7/2020 10:04 AM, Matthias Pohl wrote:
>>>>>>>>>>> I find the ratchet feature you're suggesting interesting. But
>>>> Arvid
>>>>>>>>> has a
>>>>>>>>>>> point referring to the blog post about ignoring revisions in
>> git
>>>>>>>>>>> blame
>>>>>>>>> [1].
>>>>>>>>>>> Adding the configuration file for commits to ignore revs as
>>>> proposed
>>>>>>>>> in the
>>>>>>>>>>> blog post makes it even easier. One problem I see is that
>> this is
>>>>>>>>>>> not
>>>>>>>>>>> supported by Github (yet?) [2] as mentioned in [1].
>>>>>>>>>>>
>>>>>>>>>>> Considering all that I prefer applying the code style in one
>> go. I
>>>>>>>>> have no
>>>>>>>>>>> strong opinion on what codestyle is the best.
>>>>>>>>>>>
>>>>>>>>>>> PS: We used spotless in the project I previously worked on.
>> It was
>>>>>>>>>>> convenient to use.
>>>>>>>>>>>
>>>>>>>>>>> [1]
>>>>>>>>>>>
>>>>>>>>>
>>>>>
>>>>
>> https://www.moxio.com/blog/43/ignoring-bulk-change-commits-with-git-blame
>>>>>>>>>
>>>>>>>>>>> [2]
>>>>>>>>>>>
>>>>>>>>>
>>>>>
>>>>
>> https://github.community/t/support-ignore-revs-file-in-githubs-blame-view/3256
>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> On Tue, Oct 6, 2020 at 6:00 PM Aljoscha Krettek
>>>>>>>>>>> <[hidden email]>
>>>>>>>>> wrote:
>>>>>>>>>>>
>>>>>>>>>>>> Maybe I wasn't very clear on how the ratchet works. The
>> changes
>>>> are
>>>>>>>>>>>> gradual yes, but it doesn't leave you an option: if you
>> touch a
>>>>>>>>>>>> file
>>>>>>>>> you
>>>>>>>>>>>> will it will have to conform to the coding style afterwards.
>>>>>>>>>>>> It's not
>>>>>>>>>>>> like the previous gradual process we had before where it
>> would be
>>>>>>>>> based
>>>>>>>>>>>> on people actively working towards a style.
>>>>>>>>>>>>
>>>>>>>>>>>> That being said, I also completely like the option of just
>> doing
>>>>>>>>>>>> one
>>>>>>>>> big
>>>>>>>>>>>> change commit.
>>>>>>>>>>>>
>>>>>>>>>>>> Regarding actual coding styles: we're a bit limited by what
>> tools
>>>>>>>>> exist.
>>>>>>>>>>>> I like Spotless because it can be used to both check and
>> apply a
>>>>>>>>> style.
>>>>>>>>>>>> Then you need a formatter that works with Spotless and of
>> those
>>>> we
>>>>>>>>> only
>>>>>>>>>>>> have the Eclipse Formatter, google-java-format, and Prettier.
>>>>>>>>>>>> Prettier
>>>>>>>>>>>> is a Javascript tool that I would like to avoid. Eclipse is
>>>>>>>>>>>> doable but
>>>>>>>>>>>> you need to fiddle with configuration files. I like
>>>>>>>>>>>> google-java-format
>>>>>>>>>>>> because of it's take-it-or-leave-it approach. You either use
>> the
>>>>>>>>>>>> style
>>>>>>>>>>>> or you don't but it's very thorough. The downside is that it
>>>>>>>>>>>> won't do
>>>>>>>>>>>> tabs-only formatting.
>>>>>>>>>>>>
>>>>>>>>>>>> Best,
>>>>>>>>>>>> Aljoscha
>>>>>>>>>>>>
>>>>>>>>>>>> On 06.10.20 17:43, Arvid Heise wrote:
>>>>>>>>>>>>> After having written that I did a quick search, you can even
>>>>>>>>>>>>> use git
>>>>>>>>>>>> blame
>>>>>>>>>>>>> with one big massive change commit [1], which would further
>>>>>>>>>>>>> help the
>>>>>>>>> idea
>>>>>>>>>>>>> of "just get over with it".
>>>>>>>>>>>>>
>>>>>>>>>>>>> With that option, I'd even change all whitespaces if the
>>>> community
>>>>>>>>> thinks
>>>>>>>>>>>>> that it's a better option (a separate discussion that I'll
>>>> gladly
>>>>>>>>> skip).
>>>>>>>>>>>>>
>>>>>>>>>>>>> [1]
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>
>>>>>
>>>>
>> https://www.moxio.com/blog/43/ignoring-bulk-change-commits-with-git-blame
>>>>>>>>>
>>>>>>>>>>>>> On Tue, Oct 6, 2020 at 5:38 PM Arvid Heise <
>> [hidden email]
>>>>>
>>>>>>>>> wrote:
>>>>>>>>>>>>>
>>>>>>>>>>>>>> I'm also +1 for automatically enforceable code style.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> I also would just go over it as Chesnay said. While it
>> makes
>>>> some
>>>>>>>>>>>> changes
>>>>>>>>>>>>>> a bit harder to track (inline git blame), it's easy to skip
>>>>>>>>>>>>>> over in
>>>>>>>>> any
>>>>>>>>>>>> git
>>>>>>>>>>>>>> history and if it's only one massive commit, then it's also
>>>> much
>>>>>>>>> easier
>>>>>>>>>>>> to
>>>>>>>>>>>>>> ignore than many gradual changes. Further, if we just do it
>>>> once,
>>>>>>>>> git
>>>>>>>>>>>> blame
>>>>>>>>>>>>>> will quickly become more reliable again.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Btw I completely don't care about the code style as long
>> as it
>>>>>>>>>>>>>> plays
>>>>>>>>>>>> well
>>>>>>>>>>>>>> with IntelliJ (it used to be different, but things change
>> :p).
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> On Tue, Oct 6, 2020 at 5:23 PM Chesnay Schepler
>>>>>>>>>>>>>> <[hidden email]
>>>>>>>>>>
>>>>>>>>>>>>>> wrote:
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> We shouldn't switch to spaces _now_; cutting this bit from
>>>> your
>>>>>>>>>>>> proposal
>>>>>>>>>>>>>>> will massively simplify things and there's hardly any
>> value in
>>>>>>>>> changing
>>>>>>>>>>>>>>> it.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Also I'm getting rather tired of this constant idea of
>>>> "gradual
>>>>>>>>>>>>>>> application". We've been doing this for 2-3 years now
>> since we
>>>>>>>>>>>>>>> introduced Checkstyle and basically got nowhere. We should
>>>> just
>>>>>>>>> bite
>>>>>>>>>>>> the
>>>>>>>>>>>>>>> bullet and get it over with; we could've solved this whole
>>>>>>>>>>>>>>> problem
>>>>>>>>>>>>>>> already.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> In conclusion, I'm +1 on finally locking down the
>> codestyle
>>>> and
>>>>>>>>>>>> applying
>>>>>>>>>>>>>>> it immediately, I'm -1 on any gradual application scheme
>>>> because
>>>>>>>>> they
>>>>>>>>>>>>>>> _just don't work_.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> On 10/6/2020 2:15 PM, Aljoscha Krettek wrote:
>>>>>>>>>>>>>>>> Hi All,
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> I know I know, but please keep reading because I recently
>>>>>>>>>>>>>>>> learned
>>>>>>>>>>>>>>>> about some new developments in the area of coding-style
>>>>>>>>> automation.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> The tool I would propose we use is Spotless
>>>>>>>>>>>>>>>> (https://github.com/diffplug/spotless). This doesn't
>> come
>>>>>>>>>>>>>>>> with a
>>>>>>>>>>>>>>>> formatter but allows using other popular formatters such
>> as
>>>>>>>>>>>>>>>> google-java-format. The nice thing about Spotless is
>> that it
>>>>>>>>> serves as
>>>>>>>>>>>>>>>> a verifier for CI but can also apply the configured style
>>>>>>>>>>>>>>>> automatically. That is, for the programmer all she has
>> to do
>>>> is
>>>>>>>>> `mvn
>>>>>>>>>>>>>>>> spotless:apply` to fix any style violations.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> An interesting feature, which was (somewhat) recently
>> added
>>>> is
>>>>>>>>>>>>>>>> "ratchet"
>>>>>>>>>>>>>>>> (
>>>>>>>>>>>>
>>>>>>>>>
>>>>>
>>>>
>> https://github.com/diffplug/spotless/blob/main/plugin-maven/README.md#ratchet
>>>>>>>>>
>>>>>>>>>>>> ).
>>>>>>>>>>>>>>>> With this, you can set up Spotless to only apply it's
>> rules
>>>> to
>>>>>>>>> files
>>>>>>>>>>>>>>>> that were changed after a configured commit. This would
>>>> allow a
>>>>>>>>>>>>>>>> gradual application of the new coding style instead of
>> one
>>>> big
>>>>>>>>> change.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> If we decide to use Spotless, we would of course also
>> have to
>>>>>>>>> decide
>>>>>>>>>>>>>>>> on a coding style. For this I would propose
>>>> google-java-format,
>>>>>>>>> which
>>>>>>>>>>>>>>>> the flink-statefun project uses. The main difference
>> from our
>>>>>>>>> current
>>>>>>>>>>>>>>>> "style" is that this uses spaces instead of tabs for
>>>>>>>>>>>>>>>> indentation.
>>>>>>>>> By
>>>>>>>>>>>>>>>> default it would be 2 spaces but it can be configured to
>> use
>>>> 4
>>>>>>>>> spaces
>>>>>>>>>>>>>>>> which would make code look more or less like our current
>>>> style.
>>>>>>>>> There
>>>>>>>>>>>>>>>> are no more configuration knobs, so using tabs is not an
>>>>>>>>>>>>>>>> option.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> Finally, why should we do this? I think most engineers
>> agree
>>>>>>>>>>>>>>>> that
>>>>>>>>>>>>>>>> having a common enforced style is good to have so I only
>>>>>>>>>>>>>>>> want to
>>>>>>>>>>>>>>>> highlight a few thoughts here about things we could
>> improve:
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>    - No more nits about coding style in reviews, this
>> makes
>>>> it
>>>>>>>>> easier
>>>>>>>>>>>>>>>> for both the reviewer and developer
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>    - No manual fixing of Checkstyle errors because
>> Spotless
>>>>>>>>>>>>>>>> can
>>>>>>>>> do that
>>>>>>>>>>>>>>>> automatically
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>    - Because Flink is such a big project little islands
>> of
>>>>>>>>>>>>>>>> coding
>>>>>>>>> style
>>>>>>>>>>>>>>>> have formed between people that commonly work on
>> components.
>>>> It
>>>>>>>>> can be
>>>>>>>>>>>>>>>> a nuisance when you work on a different component and
>> then
>>>>>>>>> reviewers
>>>>>>>>>>>>>>>> don't like your typical coding style. And you first have
>> to
>>>> get
>>>>>>>>> used
>>>>>>>>>>>>>>>> to the slight differences in style when reading code.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> There are also downsides I see in this:
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>    - We break the history, but both "git blame" and
>> modern
>>>>>>>>> IntelliJ can
>>>>>>>>>>>>>>>> ignore whitespace when attributing changes. So for files
>>>>>>>>>>>>>>>> that are
>>>>>>>>>>>>>>>> already "well" formatted not much would change.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>    - In the short-term it will be harder to apply
>> changes
>>>>>>>>>>>>>>>> both to
>>>>>>>>>>>> master
>>>>>>>>>>>>>>>> and one of the release-x branches because formatting
>> will be
>>>>>>>>>>>>>>>> different. I think this is not too hard though because
>>>> Spotless
>>>>>>>>> can
>>>>>>>>>>>>>>>> automatically apply the style.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> In summary, we would have some short-term pain with this
>> but
>>>> I
>>>>>>>>> think
>>>>>>>>>>>>>>>> it would be good in the long run. What are your thoughts?
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> Best,
>>>>>>>>>>>>>>>> Aljoscha
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>> --
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Arvid Heise | Senior Java Developer
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> <https://www.ververica.com/>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Follow us @VervericaData
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> --
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Join Flink Forward <https://flink-forward.org/> - The
>> Apache
>>>>>>>>>>>>>> Flink
>>>>>>>>>>>>>> Conference
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Stream Processing | Event Driven | Real Time
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> --
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Ververica GmbH | Invalidenstrasse 115, 10115 Berlin,
>> Germany
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> --
>>>>>>>>>>>>>> Ververica GmbH
>>>>>>>>>>>>>> Registered at Amtsgericht Charlottenburg: HRB 158244 B
>>>>>>>>>>>>>> Managing Directors: Timothy Alexander Steinert, Yip Park
>> Tung
>>>>>>>>> Jason, Ji
>>>>>>>>>>>>>> (Toni) Cheng
>>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>>
>>>>
>>>
>>>
>>> --
>>>
>>> Arvid Heise | Senior Java Developer
>>>
>>> <https://www.ververica.com/>
>>>
>>> Follow us @VervericaData
>>>
>>> --
>>>
>>> Join Flink Forward <https://flink-forward.org/> - The Apache Flink
>>> Conference
>>>
>>> Stream Processing | Event Driven | Real Time
>>>
>>> --
>>>
>>> Ververica GmbH | Invalidenstrasse 115, 10115 Berlin, Germany
>>>
>>> --
>>> Ververica GmbH
>>> Registered at Amtsgericht Charlottenburg: HRB 158244 B
>>> Managing Directors: Timothy Alexander Steinert, Yip Park Tung Jason, Ji
>>> (Toni) Cheng
>>

[Igal Shilman]

+1 it works really well in StateFun for quite some time.

On Thursday, December 17, 2020, Wei Zhong <[hidden email]> wrote:

> +1 for the coding style automation. Thanks for driving this!
>
> Best,
> Wei
>
> > 在 2020年12月17日，10:10，Xingbo Huang <[hidden email]> 写道：
> >
> > +1 asap. Spotless can greatly help us save the time of fixing checkstyle
> > errors.
> >
> > Best,
> > Xingbo
> >
> > Kostas Kloudas <[hidden email]> 于2020年12月17日周四 上午4:14写道：
> >
> >> +1 asap from my side as well.
> >>
> >> On Wed, Dec 16, 2020 at 8:04 PM Arvid Heise <[hidden email]>
> wrote:
> >>>
> >>> +1 asap.
> >>>
> >>> On Wed, Dec 16, 2020 at 7:44 PM Robert Metzger <[hidden email]>
> >> wrote:
> >>>
> >>>> +1
> >>>>
> >>>> Thanks for driving this.
> >>>>
> >>>> On Wed, Dec 16, 2020 at 7:33 PM Chesnay Schepler <[hidden email]>
> >>>> wrote:
> >>>>
> >>>>> +1 to set this up ASAP. Now's a good time to (finally) finalize this
> >>>>> effort with a new release cycle having started and
> >> christmas/vacations
> >>>>> being around the corner.
> >>>>>
> >>>>> On 12/16/2020 7:20 PM, Aljoscha Krettek wrote:
> >>>>>> Let's try and conclude this discussion! I've prepared a PoC that
> >> uses
> >>>>>> Spotless with google-java-format to do the formatting:
> >>>>>> https://github.com/aljoscha/flink/commits/flink-xxx-spotless
> >>>>>>
> >>>>>> To summarize from earlier discussion, the main benefits are:
> >>>>>> - no more worrying about code style, both as reviewer and
> >> implementer
> >>>>>> - everyone can configure their IDE to auto-format, there will
> >> never
> >>>>>> be unrelated formatting changes
> >>>>>>
> >>>>>> Also, this uses git's blame.ignoreRevsFile to add a file that tells
> >>>>>> git blame/IntelliJ to ignore the refactoring commit. However, you
> >> need
> >>>>>> to manually configure your git for that using:
> >>>>>>
> >>>>>> $ git config blame.ignoreRevsFile .git-blame-ignore-revs
> >>>>>>
> >>>>>> You can check out the PoC, look at the code in an IDE, play around
> >>>>>> with blame/annotations.
> >>>>>>
> >>>>>> By the way, the coding style is not configurable, it’s the Google
> >> Java
> >>>>>> Style, wich uses spaces for indentation. In an IDE or on github the
> >>>>>> code looks barely different from the previous formatting at a first
> >>>>>> glance.
> >>>>>>
> >>>>>> For IDE setup, I will add a guide in the README but it boils down
> >> to:
> >>>>>>
> >>>>>> - install the google-java-format plugin, enable it
> >>>>>> - install the Save Actions plugin, enable "Optimize Imports" and
> >>>>>> "Reformat File"
> >>>>>>
> >>>>>> With this, every time you save, formatting will be applied
> >>>>>> automatically. You will never see formatting complaints from CI
> >>>>>> (except for cases where you break semantical checkstyle rules,
> >> such as
> >>>>>> using certain imports that we don't allow.).
> >>>>>>
> >>>>>> What do you think?
> >>>>>>
> >>>>>> Best,
> >>>>>> Aljoscha
> >>>>>>
> >>>>>> On 19.10.20 12:36, Aljoscha Krettek wrote:
> >>>>>>> I don't like checkstyle because it cannot be easily applied from
> >> the
> >>>>>>> commandline. I'm happy to learn otherwise, though. And I'd also be
> >>>>>>> very happy about alternative suggestions that can do that.
> >>>>>>>
> >>>>>>> Right now, I think Spotless is the most straightforward tool for
> >>>>>>> that. Also I don't care so much about what style we choose in the
> >>>>>>> end. (If we choose one). My main motivation is that we have a
> >> common,
> >>>>>>> strict style that can easily applied via tooling so that we no
> >> longer
> >>>>>>> need to comment on coding style in PRs.
> >>>>>>>
> >>>>>>> Aljoscha
> >>>>>>>
> >>>>>>> On 09.10.20 11:11, tison wrote:
> >>>>>>>> +1 on locking on one codestyle and I think that is what current
> >>>>>>>> checkstyle
> >>>>>>>> rules serving.
> >>>>>>>>
> >>>>>>>> For automatic applying part, we suggest developing by IDEA and
> >> with
> >>>>>>>> Checkstyle Plugin on IDEA applying checkstyle suggestion is also
> >>>>>>>> automatic.
> >>>>>>>>
> >>>>>>>> One short coming for spotless is that we can hardly adjust rules
> >> if
> >>>> the
> >>>>>>>> project has its own issues to overcome. IIRC only several
> >>>>>>>> pre-defined rules
> >>>>>>>> and a clumsy general regex substitution rule can be used.
> >>>>>>>>
> >>>>>>>> FYI my team started with spotless but ended up with checkstyle
> >> with
> >>>> few
> >>>>>>>> rules and Checkstyle Plugin for automation.
> >>>>>>>>
> >>>>>>>> Codestyle, in another perspective, is part of cognition of
> >> developers
> >>>>>>>> working in project, not something we just apply before pull
> >> request.
> >>>> No
> >>>>>>>> matter how much automation introduced, most of developers will
> >>>> converge
> >>>>>>>> working with the configured codestyle.
> >>>>>>>>
> >>>>>>>> Best,
> >>>>>>>> tison.
> >>>>>>>>
> >>>>>>>>
> >>>>>>>> Kostas Kloudas <[hidden email]> 于2020年10月7日周三 下午6:37写道：
> >>>>>>>>
> >>>>>>>>> Hi all,
> >>>>>>>>>
> >>>>>>>>> +1 for enforcing "a" codestyle using "a" tool.
> >>>>>>>>>
> >>>>>>>>> As the project grows both in terms of LOCs and contributors,
> >> this
> >>>>>>>>> becomes more and more important as it eliminates some potential
> >>>> points
> >>>>>>>>> of friction without any additional effort.
> >>>>>>>>>
> >>>>>>>>> From the discussion, I am leaning towards having a single
> >> commit
> >>>> with
> >>>>>>>>> all the codestyle-related changes. This will avoid sprinkling
> >>>>>>>>> refactoring commits all over the place for the next year or
> >> more.
> >>>> But
> >>>>>>>>> if this is the price to pay for having consensus on a tool,
> >> then I
> >>>> am
> >>>>>>>>> ok with gradual implementation. I believe that the value added
> >> by
> >>>>>>>>> having an automated process of enforcing a codestyle exceeds the
> >>>> cost
> >>>>>>>>> of the nuisance of gradual refactoring.
> >>>>>>>>>
> >>>>>>>>> As for the actual format, I like the google-java-format but
> >> again,
> >>>> if
> >>>>>>>>> the community agrees on a different one I would not oppose that
> >> (as
> >>>>>>>>> long as it does not use the same amount of indentation for
> >> method
> >>>> args
> >>>>>>>>> and method body :P).
> >>>>>>>>>
> >>>>>>>>> Cheers,
> >>>>>>>>> Kostas
> >>>>>>>>>
> >>>>>>>>> On Wed, Oct 7, 2020 at 10:26 AM Chesnay Schepler <
> >>>> [hidden email]>
> >>>>>>>>> wrote:
> >>>>>>>>>>
> >>>>>>>>>> To me, ratchet seems to combine the worst aspects of both
> >>>> approaches.
> >>>>>>>>>> You get disruptive changes, but only in singular files,
> >>>>>>>>>> for something mundane as a typo fix or import change, which
> >> would
> >>>> be
> >>>>>>>>>> annoying to keep separate from the actual functional changes
> >> in a
> >>>> PR.
> >>>>>>>>>>
> >>>>>>>>>> On 10/7/2020 10:04 AM, Matthias Pohl wrote:
> >>>>>>>>>>> I find the ratchet feature you're suggesting interesting. But
> >>>> Arvid
> >>>>>>>>> has a
> >>>>>>>>>>> point referring to the blog post about ignoring revisions in
> >> git
> >>>>>>>>>>> blame
> >>>>>>>>> [1].
> >>>>>>>>>>> Adding the configuration file for commits to ignore revs as
> >>>> proposed
> >>>>>>>>> in the
> >>>>>>>>>>> blog post makes it even easier. One problem I see is that
> >> this is
> >>>>>>>>>>> not
> >>>>>>>>>>> supported by Github (yet?) [2] as mentioned in [1].
> >>>>>>>>>>>
> >>>>>>>>>>> Considering all that I prefer applying the code style in one
> >> go. I
> >>>>>>>>> have no
> >>>>>>>>>>> strong opinion on what codestyle is the best.
> >>>>>>>>>>>
> >>>>>>>>>>> PS: We used spotless in the project I previously worked on.
> >> It was
> >>>>>>>>>>> convenient to use.
> >>>>>>>>>>>
> >>>>>>>>>>> [1]
> >>>>>>>>>>>
> >>>>>>>>>
> >>>>>
> >>>>
> >> https://www.moxio.com/blog/43/ignoring-bulk-change-commits-
> with-git-blame
> >>>>>>>>>
> >>>>>>>>>>> [2]
> >>>>>>>>>>>
> >>>>>>>>>
> >>>>>
> >>>>
> >> https://github.community/t/support-ignore-revs-file-in-
> githubs-blame-view/3256
> >>>>>>>>>
> >>>>>>>>>>>
> >>>>>>>>>>> On Tue, Oct 6, 2020 at 6:00 PM Aljoscha Krettek
> >>>>>>>>>>> <[hidden email]>
> >>>>>>>>> wrote:
> >>>>>>>>>>>
> >>>>>>>>>>>> Maybe I wasn't very clear on how the ratchet works. The
> >> changes
> >>>> are
> >>>>>>>>>>>> gradual yes, but it doesn't leave you an option: if you
> >> touch a
> >>>>>>>>>>>> file
> >>>>>>>>> you
> >>>>>>>>>>>> will it will have to conform to the coding style afterwards.
> >>>>>>>>>>>> It's not
> >>>>>>>>>>>> like the previous gradual process we had before where it
> >> would be
> >>>>>>>>> based
> >>>>>>>>>>>> on people actively working towards a style.
> >>>>>>>>>>>>
> >>>>>>>>>>>> That being said, I also completely like the option of just
> >> doing
> >>>>>>>>>>>> one
> >>>>>>>>> big
> >>>>>>>>>>>> change commit.
> >>>>>>>>>>>>
> >>>>>>>>>>>> Regarding actual coding styles: we're a bit limited by what
> >> tools
> >>>>>>>>> exist.
> >>>>>>>>>>>> I like Spotless because it can be used to both check and
> >> apply a
> >>>>>>>>> style.
> >>>>>>>>>>>> Then you need a formatter that works with Spotless and of
> >> those
> >>>> we
> >>>>>>>>> only
> >>>>>>>>>>>> have the Eclipse Formatter, google-java-format, and Prettier.
> >>>>>>>>>>>> Prettier
> >>>>>>>>>>>> is a Javascript tool that I would like to avoid. Eclipse is
> >>>>>>>>>>>> doable but
> >>>>>>>>>>>> you need to fiddle with configuration files. I like
> >>>>>>>>>>>> google-java-format
> >>>>>>>>>>>> because of it's take-it-or-leave-it approach. You either use
> >> the
> >>>>>>>>>>>> style
> >>>>>>>>>>>> or you don't but it's very thorough. The downside is that it
> >>>>>>>>>>>> won't do
> >>>>>>>>>>>> tabs-only formatting.
> >>>>>>>>>>>>
> >>>>>>>>>>>> Best,
> >>>>>>>>>>>> Aljoscha
> >>>>>>>>>>>>
> >>>>>>>>>>>> On 06.10.20 17:43, Arvid Heise wrote:
> >>>>>>>>>>>>> After having written that I did a quick search, you can even
> >>>>>>>>>>>>> use git
> >>>>>>>>>>>> blame
> >>>>>>>>>>>>> with one big massive change commit [1], which would further
> >>>>>>>>>>>>> help the
> >>>>>>>>> idea
> >>>>>>>>>>>>> of "just get over with it".
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> With that option, I'd even change all whitespaces if the
> >>>> community
> >>>>>>>>> thinks
> >>>>>>>>>>>>> that it's a better option (a separate discussion that I'll
> >>>> gladly
> >>>>>>>>> skip).
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> [1]
> >>>>>>>>>>>>>
> >>>>>>>>>>>>
> >>>>>>>>>
> >>>>>
> >>>>
> >> https://www.moxio.com/blog/43/ignoring-bulk-change-commits-
> with-git-blame
> >>>>>>>>>
> >>>>>>>>>>>>> On Tue, Oct 6, 2020 at 5:38 PM Arvid Heise <
> >> [hidden email]
> >>>>>
> >>>>>>>>> wrote:
> >>>>>>>>>>>>>
> >>>>>>>>>>>>>> I'm also +1 for automatically enforceable code style.
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>> I also would just go over it as Chesnay said. While it
> >> makes
> >>>> some
> >>>>>>>>>>>> changes
> >>>>>>>>>>>>>> a bit harder to track (inline git blame), it's easy to skip
> >>>>>>>>>>>>>> over in
> >>>>>>>>> any
> >>>>>>>>>>>> git
> >>>>>>>>>>>>>> history and if it's only one massive commit, then it's also
> >>>> much
> >>>>>>>>> easier
> >>>>>>>>>>>> to
> >>>>>>>>>>>>>> ignore than many gradual changes. Further, if we just do it
> >>>> once,
> >>>>>>>>> git
> >>>>>>>>>>>> blame
> >>>>>>>>>>>>>> will quickly become more reliable again.
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>> Btw I completely don't care about the code style as long
> >> as it
> >>>>>>>>>>>>>> plays
> >>>>>>>>>>>> well
> >>>>>>>>>>>>>> with IntelliJ (it used to be different, but things change
> >> :p).
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>> On Tue, Oct 6, 2020 at 5:23 PM Chesnay Schepler
> >>>>>>>>>>>>>> <[hidden email]
> >>>>>>>>>>
> >>>>>>>>>>>>>> wrote:
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>>> We shouldn't switch to spaces _now_; cutting this bit from
> >>>> your
> >>>>>>>>>>>> proposal
> >>>>>>>>>>>>>>> will massively simplify things and there's hardly any
> >> value in
> >>>>>>>>> changing
> >>>>>>>>>>>>>>> it.
> >>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>> Also I'm getting rather tired of this constant idea of
> >>>> "gradual
> >>>>>>>>>>>>>>> application". We've been doing this for 2-3 years now
> >> since we
> >>>>>>>>>>>>>>> introduced Checkstyle and basically got nowhere. We should
> >>>> just
> >>>>>>>>> bite
> >>>>>>>>>>>> the
> >>>>>>>>>>>>>>> bullet and get it over with; we could've solved this whole
> >>>>>>>>>>>>>>> problem
> >>>>>>>>>>>>>>> already.
> >>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>> In conclusion, I'm +1 on finally locking down the
> >> codestyle
> >>>> and
> >>>>>>>>>>>> applying
> >>>>>>>>>>>>>>> it immediately, I'm -1 on any gradual application scheme
> >>>> because
> >>>>>>>>> they
> >>>>>>>>>>>>>>> _just don't work_.
> >>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>> On 10/6/2020 2:15 PM, Aljoscha Krettek wrote:
> >>>>>>>>>>>>>>>> Hi All,
> >>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>> I know I know, but please keep reading because I recently
> >>>>>>>>>>>>>>>> learned
> >>>>>>>>>>>>>>>> about some new developments in the area of coding-style
> >>>>>>>>> automation.
> >>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>> The tool I would propose we use is Spotless
> >>>>>>>>>>>>>>>> (https://github.com/diffplug/spotless). This doesn't
> >> come
> >>>>>>>>>>>>>>>> with a
> >>>>>>>>>>>>>>>> formatter but allows using other popular formatters such
> >> as
> >>>>>>>>>>>>>>>> google-java-format. The nice thing about Spotless is
> >> that it
> >>>>>>>>> serves as
> >>>>>>>>>>>>>>>> a verifier for CI but can also apply the configured style
> >>>>>>>>>>>>>>>> automatically. That is, for the programmer all she has
> >> to do
> >>>> is
> >>>>>>>>> `mvn
> >>>>>>>>>>>>>>>> spotless:apply` to fix any style violations.
> >>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>> An interesting feature, which was (somewhat) recently
> >> added
> >>>> is
> >>>>>>>>>>>>>>>> "ratchet"
> >>>>>>>>>>>>>>>> (
> >>>>>>>>>>>>
> >>>>>>>>>
> >>>>>
> >>>>
> >> https://github.com/diffplug/spotless/blob/main/plugin-
> maven/README.md#ratchet
> >>>>>>>>>
> >>>>>>>>>>>> ).
> >>>>>>>>>>>>>>>> With this, you can set up Spotless to only apply it's
> >> rules
> >>>> to
> >>>>>>>>> files
> >>>>>>>>>>>>>>>> that were changed after a configured commit. This would
> >>>> allow a
> >>>>>>>>>>>>>>>> gradual application of the new coding style instead of
> >> one
> >>>> big
> >>>>>>>>> change.
> >>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>> If we decide to use Spotless, we would of course also
> >> have to
> >>>>>>>>> decide
> >>>>>>>>>>>>>>>> on a coding style. For this I would propose
> >>>> google-java-format,
> >>>>>>>>> which
> >>>>>>>>>>>>>>>> the flink-statefun project uses. The main difference
> >> from our
> >>>>>>>>> current
> >>>>>>>>>>>>>>>> "style" is that this uses spaces instead of tabs for
> >>>>>>>>>>>>>>>> indentation.
> >>>>>>>>> By
> >>>>>>>>>>>>>>>> default it would be 2 spaces but it can be configured to
> >> use
> >>>> 4
> >>>>>>>>> spaces
> >>>>>>>>>>>>>>>> which would make code look more or less like our current
> >>>> style.
> >>>>>>>>> There
> >>>>>>>>>>>>>>>> are no more configuration knobs, so using tabs is not an
> >>>>>>>>>>>>>>>> option.
> >>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>> Finally, why should we do this? I think most engineers
> >> agree
> >>>>>>>>>>>>>>>> that
> >>>>>>>>>>>>>>>> having a common enforced style is good to have so I only
> >>>>>>>>>>>>>>>> want to
> >>>>>>>>>>>>>>>> highlight a few thoughts here about things we could
> >> improve:
> >>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>    - No more nits about coding style in reviews, this
> >> makes
> >>>> it
> >>>>>>>>> easier
> >>>>>>>>>>>>>>>> for both the reviewer and developer
> >>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>    - No manual fixing of Checkstyle errors because
> >> Spotless
> >>>>>>>>>>>>>>>> can
> >>>>>>>>> do that
> >>>>>>>>>>>>>>>> automatically
> >>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>    - Because Flink is such a big project little islands
> >> of
> >>>>>>>>>>>>>>>> coding
> >>>>>>>>> style
> >>>>>>>>>>>>>>>> have formed between people that commonly work on
> >> components.
> >>>> It
> >>>>>>>>> can be
> >>>>>>>>>>>>>>>> a nuisance when you work on a different component and
> >> then
> >>>>>>>>> reviewers
> >>>>>>>>>>>>>>>> don't like your typical coding style. And you first have
> >> to
> >>>> get
> >>>>>>>>> used
> >>>>>>>>>>>>>>>> to the slight differences in style when reading code.
> >>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>> There are also downsides I see in this:
> >>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>    - We break the history, but both "git blame" and
> >> modern
> >>>>>>>>> IntelliJ can
> >>>>>>>>>>>>>>>> ignore whitespace when attributing changes. So for files
> >>>>>>>>>>>>>>>> that are
> >>>>>>>>>>>>>>>> already "well" formatted not much would change.
> >>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>    - In the short-term it will be harder to apply
> >> changes
> >>>>>>>>>>>>>>>> both to
> >>>>>>>>>>>> master
> >>>>>>>>>>>>>>>> and one of the release-x branches because formatting
> >> will be
> >>>>>>>>>>>>>>>> different. I think this is not too hard though because
> >>>> Spotless
> >>>>>>>>> can
> >>>>>>>>>>>>>>>> automatically apply the style.
> >>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>> In summary, we would have some short-term pain with this
> >> but
> >>>> I
> >>>>>>>>> think
> >>>>>>>>>>>>>>>> it would be good in the long run. What are your thoughts?
> >>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>> Best,
> >>>>>>>>>>>>>>>> Aljoscha
> >>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>
> >>>>>>>>>>>>>> --
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>> Arvid Heise | Senior Java Developer
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>> <https://www.ververica.com/>
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>> Follow us @VervericaData
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>> --
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>> Join Flink Forward <https://flink-forward.org/> - The
> >> Apache
> >>>>>>>>>>>>>> Flink
> >>>>>>>>>>>>>> Conference
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>> Stream Processing | Event Driven | Real Time
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>> --
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>> Ververica GmbH | Invalidenstrasse 115, 10115 Berlin,
> >> Germany
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>> --
> >>>>>>>>>>>>>> Ververica GmbH
> >>>>>>>>>>>>>> Registered at Amtsgericht Charlottenburg: HRB 158244 B
> >>>>>>>>>>>>>> Managing Directors: Timothy Alexander Steinert, Yip Park
> >> Tung
> >>>>>>>>> Jason, Ji
> >>>>>>>>>>>>>> (Toni) Cheng
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>
> >>>>>>>>
> >>>>>>>
> >>>>>>
> >>>>>>
> >>>>>
> >>>>>
> >>>>
> >>>
> >>>
> >>> --
> >>>
> >>> Arvid Heise | Senior Java Developer
> >>>
> >>> <https://www.ververica.com/>
> >>>
> >>> Follow us @VervericaData
> >>>
> >>> --
> >>>
> >>> Join Flink Forward <https://flink-forward.org/> - The Apache Flink
> >>> Conference
> >>>
> >>> Stream Processing | Event Driven | Real Time
> >>>
> >>> --
> >>>
> >>> Ververica GmbH | Invalidenstrasse 115, 10115 Berlin, Germany
> >>>
> >>> --
> >>> Ververica GmbH
> >>> Registered at Amtsgericht Charlottenburg: HRB 158244 B
> >>> Managing Directors: Timothy Alexander Steinert, Yip Park Tung Jason, Ji
> >>> (Toni) Cheng
> >>
>
>