The formatting issues are definitely a problem with getting in new contributions. I'm in favor of any switch over that makes all style rules automatic.
On Fri, Jul 8, 2022 at 1: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 > > >
