Re: [DISCUSS] Automatic Code Formatting / Code Style / Enforcing Code Style

[Eduard Tudenhoefner]

Hey everyone,

Google Java Format + Spotless integration have been merged as part of
https://github.com/apache/iceberg/pull/5266.

The big bang reformatting (aka *spotlessApply*) will happen as part of
https://github.com/apache/iceberg/pull/5312 and once this PR is merged,
please perform the following steps if you have open PRs:

1. Rebase your PR branch against the commit before the „Big Bang“
2. Run `*./gradlew spotlessApply*`
3. Squash all commits
4. Rebase against the latest *HEAD* of the master branch
5. Force-push your branch

This should hopefully produce a minimum of conflicts on PRs.

Additionally, make sure to install the *google-java-format* plugin for your
IDE as outlined in https://github.com/apache/iceberg-docs/pull/125 so that
you can format code inside the IDE as well.

Eduard

On Fri, Jul 15, 2022 at 12:45 PM Eduard Tudenhoefner 
wrote:

> That's correct Ryan. The Google style is what can be applied across tools
> and Spotless is the tool that we use to check and *apply* that style (and
> also auto-apply the license header). The *apply* part is the important
> piece that Checkstyle for example is missing (as it's tedious to manually
> apply what *checkstyle:check* complains about).
>
> Additionally, Spotless
>  supports a
> bunch of other languages, so it would even be possible to use it for
> auto-formatting Gradle files (or Scala files using scalafmt) if we wanted
> to, but for now I would probably focus on the Java codebase.
>
> Eduard
>
> On Thu, Jul 14, 2022 at 6:58 PM Ryan Blue  wrote:
>
>> Okay, I understand. So it isn't that spotless is supported across tools,
>> it is that google style is supported and we can use Spotless to both check
>> and apply that style. Is that correct?
>>
>> On Thu, Jul 14, 2022 at 9:56 AM Eduard Tudenhoefner 
>> wrote:
>>
>>> Yeah it's how Dmitri said, the Google style is most likely on purpose
>>> not configurable, because otherwise it's getting more difficult to achieve
>>> the same formatting results across IDEs & CMD line.
>>>
>>>
>>> On Wed, Jul 13, 2022 at 7:39 PM Dmitri Bourlatchkov <
>>> dmitri.bourlatch...@dremio.com> wrote:
>>>
 Yeah, the Google style has very specific wrapping limits and javadoc
 format. It is not configurable, if we go with it.

 From my POV, using another style would be fine. However, with a custom
 style it can be hard and tricky to ensure that all IDEs enforce it the same
 way as spotless.

 The Google style has a plugin at least for IntelliJ (and likely for
 Eclipse too) so it will behave the same way in IDE and cmd. line build.

 On Wed, Jul 13, 2022 at 1:28 PM Ryan Blue  wrote:

> Is there any value in trying to get Spotless to match the current
> format as closely as possible? It would be great to have fewer changes.
>
> For example, when I tested the apply, I see this:
>
> -  /**
> -   * Returns an initialized {@link AliyunProperties}
> -   */
> +  /** Returns an initialized {@link AliyunProperties} */
>
> Changes like that don’t seem very valuable to me. Similarly, it looks
> like a lot of text is wrapped at a different line length. If we can alter
> that, we could easily avoid a lot of changes.
>
> Ryan
>
> On Wed, Jul 13, 2022 at 7:41 AM Eduard Tudenhoefner 
> wrote:
>
>> As a first step, I created
>> https://github.com/apache/iceberg/pull/5266 which configures
>> Spotless to use the Google Java Format and also apply the correct 
>> copyright
>> header for Java files.
>>
>> Once this PR is merged, the next steps would be:
>>
>>- removing conflicting Checkstyle rules that are not in line with
>>the Google format
>>- formatting the entire code base via `*./gradlew spotlessApply*`
>>- setting `*enforceCheck*` to `*true*` in
>>
>> https://github.com/apache/iceberg/blob/80318d8cfbeb0d96d0afc27c84bc3dbddde35344/baseline.gradle#L48
>>so that validation fails if code isn't properly formatted
>>- updating docs around the current Formatter usage and how to
>>configure Eclipse/IntelliJ
>>
>> The first 3 steps should be done together as part of the big bang.
>>
>> Eduard
>>
>> On Mon, Jul 11, 2022 at 10:54 PM Ryan Blue  wrote:
>>
>>> Okay, it sounds like there's mostly agreement for going with
>>> spotless. Let's try that out. We'll work on some changes to add 
>>> spotless so
>>> that `spotlessApply` works. Then we can do the big bang migration 
>>> (which I
>>> also agree is the best option) just before the 1.0.
>>>
>>> Thanks, everyone!
>>>
>>> On Mon, Jul 11, 2022 at 11:50 AM Dmitri Bourlatchkov <
>>> dmitri.bourlatch...@dremio.com> wrote:
>>>
 My experience with the Google Code Style + Spotless was positive
 too.

 I'd be fine with anot

Re: [DISCUSS] Automatic Code Formatting / Code Style / Enforcing Code Style

[Steven Wu]

Eduard, thank you for driving this effort! Great work!

On Wed, Jul 27, 2022 at 11:11 AM Eduard Tudenhoefner 
wrote:

> Hey everyone,
>
> Google Java Format + Spotless integration have been merged as part of
> https://github.com/apache/iceberg/pull/5266.
>
> The big bang reformatting (aka *spotlessApply*) will happen as part of
> https://github.com/apache/iceberg/pull/5312 and once this PR is merged,
> please perform the following steps if you have open PRs:
>
> 1. Rebase your PR branch against the commit before the „Big Bang“
> 2. Run `*./gradlew spotlessApply*`
> 3. Squash all commits
> 4. Rebase against the latest *HEAD* of the master branch
> 5. Force-push your branch
>
> This should hopefully produce a minimum of conflicts on PRs.
>
> Additionally, make sure to install the *google-java-format* plugin for
> your IDE as outlined in https://github.com/apache/iceberg-docs/pull/125
> so that you can format code inside the IDE as well.
>
> Eduard
>
> On Fri, Jul 15, 2022 at 12:45 PM Eduard Tudenhoefner 
> wrote:
>
>> That's correct Ryan. The Google style is what can be applied across tools
>> and Spotless is the tool that we use to check and *apply* that style
>> (and also auto-apply the license header). The *apply* part is the
>> important piece that Checkstyle for example is missing (as it's tedious to
>> manually apply what *checkstyle:check* complains about).
>>
>> Additionally, Spotless
>>  supports
>> a bunch of other languages, so it would even be possible to use it for
>> auto-formatting Gradle files (or Scala files using scalafmt) if we wanted
>> to, but for now I would probably focus on the Java codebase.
>>
>> Eduard
>>
>> On Thu, Jul 14, 2022 at 6:58 PM Ryan Blue  wrote:
>>
>>> Okay, I understand. So it isn't that spotless is supported across tools,
>>> it is that google style is supported and we can use Spotless to both check
>>> and apply that style. Is that correct?
>>>
>>> On Thu, Jul 14, 2022 at 9:56 AM Eduard Tudenhoefner 
>>> wrote:
>>>
 Yeah it's how Dmitri said, the Google style is most likely on purpose
 not configurable, because otherwise it's getting more difficult to achieve
 the same formatting results across IDEs & CMD line.


 On Wed, Jul 13, 2022 at 7:39 PM Dmitri Bourlatchkov <
 dmitri.bourlatch...@dremio.com> wrote:

> Yeah, the Google style has very specific wrapping limits and javadoc
> format. It is not configurable, if we go with it.
>
> From my POV, using another style would be fine. However, with a custom
> style it can be hard and tricky to ensure that all IDEs enforce it the 
> same
> way as spotless.
>
> The Google style has a plugin at least for IntelliJ (and likely for
> Eclipse too) so it will behave the same way in IDE and cmd. line build.
>
> On Wed, Jul 13, 2022 at 1:28 PM Ryan Blue  wrote:
>
>> Is there any value in trying to get Spotless to match the current
>> format as closely as possible? It would be great to have fewer changes.
>>
>> For example, when I tested the apply, I see this:
>>
>> -  /**
>> -   * Returns an initialized {@link AliyunProperties}
>> -   */
>> +  /** Returns an initialized {@link AliyunProperties} */
>>
>> Changes like that don’t seem very valuable to me. Similarly, it looks
>> like a lot of text is wrapped at a different line length. If we can alter
>> that, we could easily avoid a lot of changes.
>>
>> Ryan
>>
>> On Wed, Jul 13, 2022 at 7:41 AM Eduard Tudenhoefner <
>> edu...@tabular.io> wrote:
>>
>>> As a first step, I created
>>> https://github.com/apache/iceberg/pull/5266 which configures
>>> Spotless to use the Google Java Format and also apply the correct 
>>> copyright
>>> header for Java files.
>>>
>>> Once this PR is merged, the next steps would be:
>>>
>>>- removing conflicting Checkstyle rules that are not in line
>>>with the Google format
>>>- formatting the entire code base via `*./gradlew spotlessApply*`
>>>- setting `*enforceCheck*` to `*true*` in
>>>
>>> https://github.com/apache/iceberg/blob/80318d8cfbeb0d96d0afc27c84bc3dbddde35344/baseline.gradle#L48
>>>so that validation fails if code isn't properly formatted
>>>- updating docs around the current Formatter usage and how to
>>>configure Eclipse/IntelliJ
>>>
>>> The first 3 steps should be done together as part of the big bang.
>>>
>>> Eduard
>>>
>>> On Mon, Jul 11, 2022 at 10:54 PM Ryan Blue  wrote:
>>>
 Okay, it sounds like there's mostly agreement for going with
 spotless. Let's try that out. We'll work on some changes to add 
 spotless so
 that `spotlessApply` works. Then we can do the big bang migration 
 (which I
 also agree is the best option) just before the 1.0.
>>

Re: [DISCUSS] Automatic Code Formatting / Code Style / Enforcing Code Style

[Eduard]

Unsubscribe

On Wed, Jul 27, 2022 at 11:11 Eduard Tudenhoefner  wrote:

> Hey everyone,
>
> Google Java Format + Spotless integration have been merged as part of
> https://github.com/apache/iceberg/pull/5266.
>
> The big bang reformatting (aka *spotlessApply*) will happen as part of
> https://github.com/apache/iceberg/pull/5312 and once this PR is merged,
> please perform the following steps if you have open PRs:
>
> 1. Rebase your PR branch against the commit before the „Big Bang“
> 2. Run `*./gradlew spotlessApply*`
> 3. Squash all commits
> 4. Rebase against the latest *HEAD* of the master branch
> 5. Force-push your branch
>
> This should hopefully produce a minimum of conflicts on PRs.
>
> Additionally, make sure to install the *google-java-format* plugin for
> your IDE as outlined in https://github.com/apache/iceberg-docs/pull/125
> so that you can format code inside the IDE as well.
>
> Eduard
>
> On Fri, Jul 15, 2022 at 12:45 PM Eduard Tudenhoefner 
> wrote:
>
>> That's correct Ryan. The Google style is what can be applied across tools
>> and Spotless is the tool that we use to check and *apply* that style
>> (and also auto-apply the license header). The *apply* part is the
>> important piece that Checkstyle for example is missing (as it's tedious to
>> manually apply what *checkstyle:check* complains about).
>>
>> Additionally, Spotless
>>  supports
>> a bunch of other languages, so it would even be possible to use it for
>> auto-formatting Gradle files (or Scala files using scalafmt) if we wanted
>> to, but for now I would probably focus on the Java codebase.
>>
>> Eduard
>>
>> On Thu, Jul 14, 2022 at 6:58 PM Ryan Blue  wrote:
>>
>>> Okay, I understand. So it isn't that spotless is supported across tools,
>>> it is that google style is supported and we can use Spotless to both check
>>> and apply that style. Is that correct?
>>>
>>> On Thu, Jul 14, 2022 at 9:56 AM Eduard Tudenhoefner 
>>> wrote:
>>>
 Yeah it's how Dmitri said, the Google style is most likely on purpose
 not configurable, because otherwise it's getting more difficult to achieve
 the same formatting results across IDEs & CMD line.


 On Wed, Jul 13, 2022 at 7:39 PM Dmitri Bourlatchkov <
 dmitri.bourlatch...@dremio.com> wrote:

> Yeah, the Google style has very specific wrapping limits and javadoc
> format. It is not configurable, if we go with it.
>
> From my POV, using another style would be fine. However, with a custom
> style it can be hard and tricky to ensure that all IDEs enforce it the 
> same
> way as spotless.
>
> The Google style has a plugin at least for IntelliJ (and likely for
> Eclipse too) so it will behave the same way in IDE and cmd. line build.
>
> On Wed, Jul 13, 2022 at 1:28 PM Ryan Blue  wrote:
>
>> Is there any value in trying to get Spotless to match the current
>> format as closely as possible? It would be great to have fewer changes.
>>
>> For example, when I tested the apply, I see this:
>>
>> -  /**
>> -   * Returns an initialized {@link AliyunProperties}
>> -   */
>> +  /** Returns an initialized {@link AliyunProperties} */
>>
>> Changes like that don’t seem very valuable to me. Similarly, it looks
>> like a lot of text is wrapped at a different line length. If we can alter
>> that, we could easily avoid a lot of changes.
>>
>> Ryan
>>
>> On Wed, Jul 13, 2022 at 7:41 AM Eduard Tudenhoefner <
>> edu...@tabular.io> wrote:
>>
>>> As a first step, I created
>>> https://github.com/apache/iceberg/pull/5266 which configures
>>> Spotless to use the Google Java Format and also apply the correct 
>>> copyright
>>> header for Java files.
>>>
>>> Once this PR is merged, the next steps would be:
>>>
>>>- removing conflicting Checkstyle rules that are not in line
>>>with the Google format
>>>- formatting the entire code base via `*./gradlew spotlessApply*`
>>>- setting `*enforceCheck*` to `*true*` in
>>>
>>> https://github.com/apache/iceberg/blob/80318d8cfbeb0d96d0afc27c84bc3dbddde35344/baseline.gradle#L48
>>>so that validation fails if code isn't properly formatted
>>>- updating docs around the current Formatter usage and how to
>>>configure Eclipse/IntelliJ
>>>
>>> The first 3 steps should be done together as part of the big bang.
>>>
>>> Eduard
>>>
>>> On Mon, Jul 11, 2022 at 10:54 PM Ryan Blue  wrote:
>>>
 Okay, it sounds like there's mostly agreement for going with
 spotless. Let's try that out. We'll work on some changes to add 
 spotless so
 that `spotlessApply` works. Then we can do the big bang migration 
 (which I
 also agree is the best option) just before the 1.0.

 Thanks, everyone!

>>>

Re: [DISCUSS] Automatic Code Formatting / Code Style / Enforcing Code Style

[Anton Okolnychyi]

I am late to the discussion, but I would like to express my opinion.

As someone who deeply cares about the code consistency and leaves a lot of 
style-related nits, I am definitely happy to see efforts to simplify the life 
of the reviewers and automate the formatting process. Explaining the code style 
to new contributors consumes a lot of time and results in more iterations on 
PRs. Automating this would be great!

However, I am concerned regarding the changes merged in PR #5312.

- Many changes that went in that PR were against the original code style of the 
project that we, committers and contributors, tried to maintain for all these 
years. I think we had a lot of well-structured and consistent places, which 
attracted devs to Iceberg.
- I feel the code quality dropped as some places were written in the old style 
and now look really weird. If we followed the new style originally, maybe, it 
would look much better as we would structure the code differently.
- As a someone whose team maintains an internal fork that is very close to 
master (I know we are not the only ones), I feel such a massive change will be 
a challenge to cherry-pick. I am not convinced introducing automatic formatting 
had to be so radical.

The first two points concern me the most. Since I am late to the discussion, it 
would not be fair for me to request changes. However, I strongly suggest the 
community to reconsider the approach we took while we haven’t merged many 
changes on top of PR #5312. 

To sum up, I definitely support automating the formatting to simplify reviews 
and help new contributors but I would try to come with a way to make it less 
radical and follow whatever we had all these years. We did discuss some of the 
formatting guidelines a few years ago and it is all different now.

- Anton

> On Jul 27, 2022, at 11:40 AM, Steven Wu  wrote:
> 
> Eduard, thank you for driving this effort! Great work!
> 
> On Wed, Jul 27, 2022 at 11:11 AM Eduard Tudenhoefner  > wrote:
> Hey everyone, 
> 
> Google Java Format + Spotless integration have been merged as part of 
> https://github.com/apache/iceberg/pull/5266 
> .
> 
> The big bang reformatting (aka spotlessApply) will happen as part of 
> https://github.com/apache/iceberg/pull/5312 
>  and once this PR is merged, 
> please perform the following steps if you have open PRs: 
> 
> 1. Rebase your PR branch against the commit before the „Big Bang“
> 2. Run `./gradlew spotlessApply`
> 3. Squash all commits
> 4. Rebase against the latest HEAD of the master branch
> 5. Force-push your branch
> 
> This should hopefully produce a minimum of conflicts on PRs.
> 
> Additionally, make sure to install the google-java-format plugin for your IDE 
> as outlined in https://github.com/apache/iceberg-docs/pull/125 
>  so that you can format code 
> inside the IDE as well.
> 
> Eduard
> 
> On Fri, Jul 15, 2022 at 12:45 PM Eduard Tudenhoefner  > wrote:
> That's correct Ryan. The Google style is what can be applied across tools and 
> Spotless is the tool that we use to check and apply that style (and also 
> auto-apply the license header). The apply part is the important piece that 
> Checkstyle for example is missing (as it's tedious to manually apply what 
> checkstyle:check complains about).
> 
> Additionally, Spotless 
>  supports a 
> bunch of other languages, so it would even be possible to use it for 
> auto-formatting Gradle files (or Scala files using scalafmt) if we wanted to, 
> but for now I would probably focus on the Java codebase.
> 
> Eduard
> 
> On Thu, Jul 14, 2022 at 6:58 PM Ryan Blue  > wrote:
> Okay, I understand. So it isn't that spotless is supported across tools, it 
> is that google style is supported and we can use Spotless to both check and 
> apply that style. Is that correct?
> 
> On Thu, Jul 14, 2022 at 9:56 AM Eduard Tudenhoefner  > wrote:
> Yeah it's how Dmitri said, the Google style is most likely on purpose not 
> configurable, because otherwise it's getting more difficult to achieve the 
> same formatting results across IDEs & CMD line. 
> 
> 
> On Wed, Jul 13, 2022 at 7:39 PM Dmitri Bourlatchkov 
> mailto:dmitri.bourlatch...@dremio.com>> 
> wrote:
> Yeah, the Google style has very specific wrapping limits and javadoc format. 
> It is not configurable, if we go with it.
> 
> From my POV, using another style would be fine. However, with a custom style 
> it can be hard and tricky to ensure that all IDEs enforce it the same way as 
> spotless.
> 
> The Google style has a plugin at least for IntelliJ (and likely for Eclipse 
> too) so it will behave the same way in IDE and cmd. line build.
> 
> On Wed, Jul 13, 2022 at 1:28 PM Ryan Blue