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
>
