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

Customize artifact of google java format #944

Merged
merged 1 commit into from
Sep 27, 2021

Conversation

figroc
Copy link
Contributor

@figroc figroc commented Sep 22, 2021

Add artifact option to GoogleJavaFormatStep.

The Google Java Format is not friendly for lambda. I've managed a customized artifact to improve it. This PR allows user to integrate home-brewed GJF with spotless.

@figroc figroc changed the title feat: customize artifact for google java format Customize artifact of google java format Sep 22, 2021
Copy link
Member

@nedtwigg nedtwigg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Happy to merge this, needs:

  • misc changes which I marked up inline
  • a teensy callout within the README for plugin-gradle and plugin-maven

@figroc
Copy link
Contributor Author

figroc commented Sep 22, 2021

@nedtwigg Thank you for your insight.

Should I keep the MAVEN_COORDINATE naming?

@nedtwigg
Copy link
Member

Yes please :)

@figroc
Copy link
Contributor Author

figroc commented Sep 22, 2021

@nedtwigg Would you take another look? groupArtifact is used instead of mavenCoordinate.

Copy link
Member

@nedtwigg nedtwigg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a few nits and I think this can get merged. You need it, and that's good enough for it to get merged.

What I don't like about it is that I think this feature is very niche, and not many people will need it. In general, I think it's good to gate stuff like that behind a custom name to indicate "you probably don't need this if you're just exploring". But maybe I'm wrong, and there are more forked GJF versions out there than I realize. So I'm okay with the naming and API as-is.

I'd like to let this cool off for 48 hrs to see if any other maintainers want to weigh-in, then I'll merge and release.

plugin-maven/README.md Outdated Show resolved Hide resolved
plugin-maven/README.md Outdated Show resolved Hide resolved
@nedtwigg
Copy link
Member

This looks great, all my concerns are satisfied. I'll merge and release sometime Thursday unless somebody objects between now and then.

@figroc
Copy link
Contributor Author

figroc commented Sep 24, 2021

@nedtwigg I forgot to add test for the feature. Please wait for my update.

@figroc
Copy link
Contributor Author

figroc commented Sep 24, 2021

@nedtwigg done adding unit tests

@figroc figroc requested a review from nedtwigg September 24, 2021 11:27
@figroc
Copy link
Contributor Author

figroc commented Sep 26, 2021

@nedtwigg I've rebased the PR. It seems there is some problem with ci/circleci: assemble_testClasses cloning the repo. What should I do?

@nedtwigg
Copy link
Member

Sorry, I'm stalled on #950. You've gone your part, I'll do mine asap.

@nedtwigg
Copy link
Member

Please merge the latest origin/main into this PR. That should fix CI, at which point I can merge and release.

update change log

add new ctor of State instead of changing existed ones

change artifact to groupArtifact

check groupArtifact content

add tests for custom group artifact
@nedtwigg nedtwigg merged commit 4e30602 into diffplug:main Sep 27, 2021
@nedtwigg
Copy link
Member

Published in plugin-gradle 5.15.2 and plugin-maven 2.14.0.

@nedtwigg
Copy link
Member

FYI, palantir-java-format is a lambda-friendly fork of GJF which shipped in Spotless plugin-gradle 6.2.0 and plugin-maven 2.20.0.

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.

2 participants