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 <b...@tabular.io> 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 <b...@apache.org> 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 another code style as long as it is "deterministic"
>>>>> (e.g. does not make changes on repeated execution) and works in IntelliJ
>>>>> IDEA / Eclipse / etc.
>>>>>
>>>>> Regarding cherry-picking into older branches, I think Robert's
>>>>> suggestion can be tweaked slightly to be helpful there too:
>>>>>
>>>>> 1. Checkout old branch
>>>>> 2. Apply the new style (run gradle ...)
>>>>> 3. Cherry-pick without committing
>>>>> 4. Manually revert to old style
>>>>> 5. Commit
>>>>> 6. Reset to original branch HEAD
>>>>> 7. Cherry pick commit 5 again
>>>>>
>>>>> It's a bit lengthy and may be a tedious process, but it should allow
>>>>> applying the git-level changes mostly automatically.
>>>>>
>>>>> Cheers,
>>>>> Dmitri.
>>>>>
>>>>>
>>>>> On Fri, Jul 8, 2022 at 2:53 AM Robert Stupp <sn...@snazy.de> wrote:
>>>>>
>>>>>> From my experience, it’s a big win to have automatic code formatting.
>>>>>>
>>>>>> In projectnessie we use automatic code formatting for all languages
>>>>>> and haven’t serious issues with Spotless. It is just nice to not have to
>>>>>> bike shed about whitespaces, line breaks, brackets, etc. It was a bit of
>>>>>> discussion, because people had bad memories from past experiences with
>>>>>> automatic code formatting breaking code and introducing subtle bugs.
>>>>>>
>>>>>> I think that using code styles that „do not allow bike shedding“
>>>>>> (Google Code Style) are a very good option.
>>>>>>
>>>>>> So far none of us has seen issues with any of the Spotless code
>>>>>> formatters that we use: XML, Kotlin/Gradle, Kotlin, Antlr4, Java, Scala -
>>>>>> relying on the „standard“ settings w/o any customizations. We use this
>>>>>> piece of code, externalized into an internal Gradle plugin:
>>>>>> https://github.com/projectnessie/gradle-build-plugins/blob/main/spotless/src/main/kotlin/org/projectnessie/buildtools/spotless/SpotlessHelperPlugin.kt
>>>>>> For
>>>>>> Iceberg, it would probably be nice to have some Groovy code formatting
>>>>>> for
>>>>>> the build scripts as well.
>>>>>>
>>>>>> Sure, the migration will add some pain. IMHO the best option is a
>>>>>> „big bang“ across the whole code base, because it happens only once.
>>>>>> Migrating one module after another is a „repeated series of pains“.
>>>>>>
>>>>>> Since the result of a `./gradlew spotlessApply` is deterministic,
>>>>>> people that have open PRs could:
>>>>>> 1. Rebase their PR branch against the commit before the „Big Bang“
>>>>>> 2. Include a commit with the necessary Gradle build change (one the
>>>>>> only contains the changes to add Spotless)
>>>>>> 3. Do the `./gradlew spotlessApply`
>>>>>> 4. Squash all commits in the PR-branch
>>>>>> 5. Rebase again - against the HEAD of the master branch
>>>>>> 6. Force-push PR-branch
>>>>>> Because git is „clever enough“ to eliminate the „duplicated/unrelated
>>>>>> changes“, the final result of the above steps is just the diff with the
>>>>>> changes for the open PR.
>>>>>>
>>>>>>
>>>>>> Am 08.07.2022 um 00:59 schrieb Ryan Blue <b...@tabular.io>:
>>>>>>
>>>>>> We were just talking about this proposal internally. I think it
>>>>>> would be great to have automatic code formatting, especially since we
>>>>>> have
>>>>>> to point out a lot of changes manually. The main question is how to get
>>>>>> there without too much disruption. This came up in our discussions around
>>>>>> the upcoming 1.0 release, since that may be a good opportunity to make
>>>>>> all
>>>>>> of the code changes.
>>>>>>
>>>>>> For background, the main concern about adding something like this is
>>>>>> applying all of the changes needed to get the existing code to conform to
>>>>>> the new style. That is really disruptive because it will cause all of the
>>>>>> PRs to need to be rebased and makes it really difficult to cherry-pick
>>>>>> changes from after the code formatting happens to branches that were
>>>>>> created before code formatting. The 1.0 release makes a good opportunity
>>>>>> because we are making other changes (removing deprecations) and will
>>>>>> hopefully have people upgrading their branches to the new major version,
>>>>>> rather than cherry picking.
>>>>>>
>>>>>> This is as good a time as any to add automatic code formatting, but
>>>>>> it's up to the community: so should we refromat the project and apply
>>>>>> spotless code formatting everywhere? I'm interested to hear opinions!
>>>>>>
>>>>>> Ryan
>>>>>>
>>>>>> On Thu, Mar 10, 2022 at 3:00 AM Eduard Tudenhoefner <
>>>>>> edu...@dremio.com> wrote:
>>>>>>
>>>>>>> Hello everyone,
>>>>>>>
>>>>>>> I would like to get the discussion started around automatic code
>>>>>>> formatting + enforcing and how we get there.
>>>>>>>
>>>>>>> Currently we use Checkstyle *check* to enforce formatting. However,
>>>>>>> the problem with that is that you still have to manually do the actual
>>>>>>> formatting.
>>>>>>>
>>>>>>> What I would like to propose is the usage of *Spotless* (
>>>>>>> https://github.com/diffplug/spotless) for *checking* and *enforcing*
>>>>>>> Java code style (it can also enforce code style for Scala, Markdown, ...
>>>>>>> btw). Spotless is being used by many projects (
>>>>>>> https://github.com/search?l=gradle&q=spotless&type=Code) and comes
>>>>>>> essentially with two tasks:
>>>>>>> * *spotlessCheck*: Checks that sourcecode satisfies formatting steps
>>>>>>> * *spotlessApply*: Applies code formatting steps to sourcecode
>>>>>>> in-place
>>>>>>>
>>>>>>>
>>>>>>> *Code format*
>>>>>>>
>>>>>>> The problem with code format is that there is no single format that
>>>>>>> can satisfy the preferences of everybody. However, from my experience,
>>>>>>> once
>>>>>>> people start to use *any* code format that produces consistent
>>>>>>> results across *Eclipse**/IntelliJ/cmd line*, people stop worrying
>>>>>>> about code format details.
>>>>>>> This is also one of the reasons why the creators of Go decided to
>>>>>>> have a code formatter built-in (
>>>>>>> https://go.dev/doc/effective_go#formatting):
>>>>>>>
>>>>>>> *Formatting issues are the most contentious but the least
>>>>>>>> consequential. People can adapt to different formatting styles but it's
>>>>>>>> better if they don't have to, and less time is devoted to the topic if
>>>>>>>> everyone adheres to the same style. The problem is how to approach this
>>>>>>>> Utopia without a long prescriptive style guide.*
>>>>>>>> *With Go we take an unusual approach and let the machine take care
>>>>>>>> of most formatting issues. The gofmt program (also available as go fmt,
>>>>>>>> which operates at the package level rather than source file level)
>>>>>>>> reads a
>>>>>>>> Go program and emits the source in a standard style of indentation and
>>>>>>>> vertical alignment, retaining and if necessary reformatting comments.*
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> I would like to propose using the Google Java Format with Spotless.
>>>>>>> The reason for this format is essentially that this is a widely-adopted
>>>>>>> code format that is designed specifically for code reviews (since we're
>>>>>>> spending more time reviewing code than writing it).
>>>>>>> Additionally, it produces consistent formatting results across
>>>>>>> *Eclipse**/IntelliJ/cmd line*, which I think is another very
>>>>>>> important factor.
>>>>>>>
>>>>>>> Thus, our initial Gradle spotless configuration could look similar
>>>>>>> to the above below:
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> *pluginManager.withPlugin('com.diffplug.spotless') { spotless {
>>>>>>> // don't run spotlessCheck during gradle check task during the
>>>>>>> transition phase enforceCheck = false java { target
>>>>>>> 'src/main/java/**/*.java', 'src/test/java/**/*.java',
>>>>>>> 'src/jmh/java/**/*.java' googleJavaFormat() } }}*
>>>>>>>
>>>>>>> We don't have to use Google Java Format. Spotless also supports
>>>>>>> formatting the code with other formats, but from previous experience the
>>>>>>> Google Java Format seemed to be really the only one to produce
>>>>>>> consistent
>>>>>>> results across *Eclipse**/IntelliJ/cmd line*.
>>>>>>>
>>>>>>>
>>>>>>> *How do we get to a point where the entire codebase is properly
>>>>>>> formatted (and enforceCheck = false can be removed)?*
>>>>>>>
>>>>>>> Now this is a difficult question. Obviously we don't want to have a
>>>>>>> single *format-everything* commit, as that would affect lots of
>>>>>>> in-flight PRs.
>>>>>>>
>>>>>>> There would have to be some form of gradual formatting, for example
>>>>>>> module by module. Spotless offers something called Ratched (
>>>>>>> https://github.com/diffplug/spotless/tree/main/plugin-gradle#ratchet)
>>>>>>> that allows to enforce code format gradually (but I'm not sure this
>>>>>>> would
>>>>>>> be a good thing either).
>>>>>>>
>>>>>>> How exactly we'd like to approach this transitioning phase this is a
>>>>>>> completely separate discussion, but I feel like at least we could get
>>>>>>> the
>>>>>>> ball rolling so that we make it also easier for newcomers to contribute
>>>>>>> to
>>>>>>> the project, since it would be straightforward for them to make their
>>>>>>> PRs
>>>>>>> adhere to the code format and also save time during PR reviews.
>>>>>>>
>>>>>>>
>>>>>>> Eduard
>>>>>>>
>>>>>>
>>>>>>
>>>>>> --
>>>>>> Ryan Blue
>>>>>> Tabular
>>>>>>
>>>>>>
>>>>>>
>>>>
>>>> --
>>>> Ryan Blue
>>>>
>>>
>>
>> --
>> Ryan Blue
>> Tabular
>>
>
