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 <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 > <mailto: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 <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 > <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 > <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 > <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
