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
