Skip to content

Conversation

@tbroyer
Copy link
Contributor

@tbroyer tbroyer commented Sep 4, 2021

Add configuration option to switch the option of the googleJavaFormat step.
Default to not reflowing for backwards compatibility.

Fixes #924

Add configuration option to switch the option of the
googleJavaFormat step.
Default to not reflowing for backwards compatibility.

Fixes diffplug#924
@jbduncan
Copy link
Member

jbduncan commented Sep 4, 2021

@tbroyer Thank you very much for your contribution!

The only thing I'm questioning is, given how many parameters GoogleJavaFormatStep::create has now, if we'd benefit from replacing its new variation with a builder. @tbroyer @nedtwigg What do you think?

If we decide the answer is "no", then I wonder if it would be best for the boolean parameter to come before the Provisioner, as provisioners are a Spotless concept, and moving it to the right of the other parameters might be easiest to understand.

Other than that, this PR LGTM, so well done and thanks again!

@nedtwigg
Copy link
Member

nedtwigg commented Sep 4, 2021

I agree with @jbduncan that this would be easier to read if we had a builder. But because the list of parameters is so long, I think it's hopeless to worry about the parameter positions. I'm going to merge and release as-is, but a future refactor towards a builder would be welcome. My guess is that this will be the last feature anyone is able to add before they're forced to confront the builder mess.

@nedtwigg nedtwigg merged commit a7e93c8 into diffplug:main Sep 4, 2021
@nedtwigg
Copy link
Member

nedtwigg commented Sep 5, 2021

Released in plugin-gradle 5.15.0 and plugin-maven 2.13.0

@tbroyer tbroyer deleted the 924-googlejavaformat-reflowlongstrings branch September 5, 2021 10:36
@tbroyer
Copy link
Contributor Author

tbroyer commented Sep 5, 2021

Fwiw, tests aren't actually run with JDK 8 on CI ; causing a risk of regressions that won't be detected. Not sure how much you care about it.

@nedtwigg
Copy link
Member

nedtwigg commented Sep 6, 2021

I think we test JDK 8, but windows-only. Agreed that it's a bit hidden.

executor:
name: win/default
shell: cmd.exe
steps:
- checkout
- run:
name: install
command: choco install ojdkbuild8
- run:
name: gradlew check
command: gradlew check --build-cache -PSPOTLESS_EXCLUDE_MAVEN=true

We get enough flaky dep-resolution problems that I don't want to trigger manual retries for every cell in the win/unix vs 8/11/latest matrix.

@tbroyer
Copy link
Contributor Author

tbroyer commented Sep 6, 2021

Strange, this test should have failed for months if that was the case: ecca2b8

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

3 participants