Skip to content
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

Support google-java-format's skip-reflowing-long-strings option #929

Merged

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Google Java Format doesn't reflow long strings
3 participants