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 <https://github.com/diffplug/spotless/tree/main/plugin-gradle> 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 <b...@tabular.io> 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 <edu...@tabular.io> > 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 <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 >>>> >>> > > -- > Ryan Blue > Tabular >
