-
Notifications
You must be signed in to change notification settings - Fork 460
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement buf format
Gradle step
#1208
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR is excellent and I'm happy to merge it as-is if you're ready. My only comments are minor docs and testing nits which can be easily fixed post merge-and-release.
@Test | ||
void test() throws Exception { | ||
try (StepHarnessWithFile harness = StepHarnessWithFile.forStep(BufStep.withVersion(BufStep.defaultVersion()).create())) { | ||
harness.testResource(new File("src/main/resources/protobuf/buf/buf.proto"), "protobuf/buf/buf.proto", "protobuf/buf/buf.proto.clean"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't really matter, but it should be okay for this to be new File("foo.proto")
. If BufStep
is implemented correctly (looks to me like it is), then the non-existent "foo.proto" isn't actually used to read any content from. The only significance of a formatter that takes the actual filename should be that formatting depends on the filename, such as package-info.java
or something like that.
So it should be fine if "foo.proto" doesn't actually exist, because it's just a fake filename to pass around.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought that might be the case, but I run into issues:
java.lang.RuntimeException: > arguments: [/usr/local/bin/buf, format, /Users/andrewparmet/.../spotless/testlib/foo.proto]
> exit code: 1
> stdout: (empty)
> stderr: Failure: input /Users/andrewparmet/.../spotless/testlib/foo.proto was not found - is the directory containing this file defined in your buf.work.yaml?
String[] processArgs = args.toArray(new String[args.size() + 1]); | ||
// add an argument to the end | ||
processArgs[args.size()] = file.getAbsolutePath(); | ||
return runner.exec(input.getBytes(StandardCharsets.UTF_8), processArgs).assertExitZero(StandardCharsets.UTF_8); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bummer, we've got a problem. Your answer about the test needing the file to exist made me suspicious, so I did a little digging.
On this line you are passing the file's content on stdin
as input.getBytes(...)
but the buf command is ignoring that and just reading the file content from disk. That breaks Spotless' model - for example, any steps that get applied before buf()
will get wiped out, and our IDE integrations will also not work as expected.
Looks like this is a known issue, and they plan to add the ability to read buf
from stdin: bufbuild/buf#1035
I'd like to delay merging this until buf
adds this feature. If this delay would be a big hassle to your workflow, I'm okay merging this semi-broken feature in, with the promise that we'll stop supporting this version of buf
once they release a version which supports stdin
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Either is fine. I can use the buf-gradle-plugin in the meantime. Bummer to not be able to use spotlessApply
for everything!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the buf-gradle-plugin is already available and the buf team has accepted the feature request, I'll wait until it gets resolved rather than merge something half-broken.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't seem like the Buf team is going to be taking on stdin support anytime soon. What are your thoughts on merging this semi-broken feature? Would it cause problems for any common formatting use cases besides license headers?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, and since I opened this PR the Buf team has begun publishing a Buf binary to Maven Central. Is there an existing pattern for locating a binary that way? The buf-gradle-plugin
references it under the hood now, so it's also possible to let it be a transparent improvement here and leave the instructions here as they are.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm okay merging this as-is.
Would it cause problems for any common formatting use cases
Main thing it breaks is idempotence. Most formatters have idempotency bugs, which Spotless quietly fixes, and that won't work here. It also means that things like replace
, license
, tabsToSpaces
etc will only work if you put them after the buf step, they can't come before. Not great, but we can document it and point them to the buf issue to thumbs-up it.
publishing a Buf binary to Maven Central
I assume the binary has to be extracted from the jar? We don't have any formatters like that currently. I'd be happy to merge that in, but even happier to leave that up to some other plugin and document that in our docs. Let me know when you're ready to merge and we'll let it rip :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok - letting buf-gradle-plugin
handle that is probably best. I corrected the docs pointing to it (Buf in-housed it earlier this year) but the instructions are the same otherwise. Extracting from the jar would mean writing code to support specifying a Maven artifact as a source for a binary and I don't think I want to tackle that at the moment given that it can be added later and we're going to put this behind common configuration code anyways.
I'm happy as this is! Should I take care of those doc notes? I checked the box to let you do it if you want to just do it quickly yourself.
Released in |
…lugin to v2.40.0 (mulk/quarkus-googlecloud-jsonlogging!20) This MR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [com.diffplug.spotless:spotless-maven-plugin](https://github.com/diffplug/spotless) | build | minor | `2.39.0` -> `2.40.0` | --- ### Release Notes <details> <summary>diffplug/spotless</summary> ### [`v2.40.0`](https://github.com/diffplug/spotless/blob/HEAD/CHANGES.md#​2400---2023-07-17) ##### Added - Added support for Protobuf formatting based on [Buf](https://buf.build/). ([#​1208](diffplug/spotless#1208)) - `enum OnMatch { INCLUDE, EXCLUDE }` so that `FormatterStep.filterByContent` can not only include based on the pattern but also exclude. ([#​1749](diffplug/spotless#1749)) ##### Fixed - Update documented default `semanticSort` to `false`. ([#​1728](diffplug/spotless#1728)) ##### Changes - Bump default `cleanthat` version to latest `2.13` -> `2.17`. ([#​1734](diffplug/spotless#1734)) - Bump default `ktlint` version to latest `0.49.1` -> `0.50.0`. ([#​1741](diffplug/spotless#1741)) - Dropped support for `ktlint 0.47.x` following our policy of supporting two breaking changes at a time. - Dropped support for deprecated `useExperimental` parameter in favor of the `ktlint_experimental` property. </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Enabled. ♻ **Rebasing**: Whenever MR is behind base branch, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this MR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this MR, check this box --- This MR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNC4yNC4wIiwidXBkYXRlZEluVmVyIjoiMzQuMjQuMCJ9-->
This MR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [flow-bin](https://github.com/flowtype/flow-bin) ([changelog](https://github.com/facebook/flow/blob/master/Changelog.md)) | devDependencies | minor | [`^0.216.0` -> `^0.217.0`](https://renovatebot.com/diffs/npm/flow-bin/0.216.1/0.217.2) | | [org.liquibase:liquibase-maven-plugin](http://www.liquibase.org/liquibase-maven-plugin) ([source](https://github.com/liquibase/liquibase)) | build | minor | `4.23.2` -> `4.24.0` | | [com.diffplug.spotless:spotless-maven-plugin](https://github.com/diffplug/spotless) | build | minor | `2.39.0` -> `2.40.0` | | [io.quarkus:quarkus-maven-plugin](https://github.com/quarkusio/quarkus) | build | patch | `3.4.1` -> `3.4.2` | | [io.quarkus:quarkus-universe-bom](https://github.com/quarkusio/quarkus-platform) | import | patch | `3.4.1` -> `3.4.2` | --- ### Release Notes <details> <summary>flowtype/flow-bin</summary> ### [`v0.217.2`](flow/flow-bin@15ccd14...dc93913) [Compare Source](flow/flow-bin@15ccd14...dc93913) ### [`v0.217.1`](flow/flow-bin@6af43b3...15ccd14) [Compare Source](flow/flow-bin@6af43b3...15ccd14) ### [`v0.217.0`](flow/flow-bin@f96ca32...6af43b3) [Compare Source](flow/flow-bin@f96ca32...6af43b3) </details> <details> <summary>liquibase/liquibase</summary> ### [`v4.24.0`](https://github.com/liquibase/liquibase/blob/HEAD/changelog.txt#Liquibase-4240-is-a-major-release) [Compare Source](liquibase/liquibase@v4.23.2...v4.24.0) </details> <details> <summary>diffplug/spotless</summary> ### [`v2.40.0`](https://github.com/diffplug/spotless/blob/HEAD/CHANGES.md#​2400---2023-07-17) ##### Added - Added support for Protobuf formatting based on [Buf](https://buf.build/). ([#​1208](diffplug/spotless#1208)) - `enum OnMatch { INCLUDE, EXCLUDE }` so that `FormatterStep.filterByContent` can not only include based on the pattern but also exclude. ([#​1749](diffplug/spotless#1749)) ##### Fixed - Update documented default `semanticSort` to `false`. ([#​1728](diffplug/spotless#1728)) ##### Changes - Bump default `cleanthat` version to latest `2.13` -> `2.17`. ([#​1734](diffplug/spotless#1734)) - Bump default `ktlint` version to latest `0.49.1` -> `0.50.0`. ([#​1741](diffplug/spotless#1741)) - Dropped support for `ktlint 0.47.x` following our policy of supporting two breaking changes at a time. - Dropped support for deprecated `useExperimental` parameter in favor of the `ktlint_experimental` property. </details> <details> <summary>quarkusio/quarkus</summary> ### [`v3.4.2`](quarkusio/quarkus@3.4.1...3.4.2) [Compare Source](quarkusio/quarkus@3.4.1...3.4.2) </details> <details> <summary>quarkusio/quarkus-platform</summary> ### [`v3.4.2`](quarkusio/quarkus-platform@3.4.1...3.4.2) [Compare Source](quarkusio/quarkus-platform@3.4.1...3.4.2) </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Enabled. ♻ **Rebasing**: Whenever MR is behind base branch, or you tick the rebase/retry checkbox. 👻 **Immortal**: This MR will be recreated if closed unmerged. Get [config help](https://github.com/renovatebot/renovate/discussions) if that's undesired. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this MR, check this box --- This MR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNC4yNC4wIiwidXBkYXRlZEluVmVyIjoiMzQuMjQuMCJ9-->
Fixes #1207.
Adds a step and a Gradle Extension for formatting Protobuf using
buf format
.Open questions:
Is there an existing pattern I can copy to grab the Buf executable as part of a Gradle build? Unlike KtLint, for example, Buf is not distributed as a JAR, so I cannot use JarState. Or can JarState be used with a custom Ivy repository?I don't actually need this, as users of the buf-gradle-plugin can access its Buf executable withShould I tag these tests in a way similar toIt won't be, as in other "foreign" formatter steps.Clang
,Black
, etc.? How can I make surebuf
is available for CI tests?