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
