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

Use spotless plugin for formating #308

Merged
merged 3 commits into from
May 23, 2021

Conversation

rnett
Copy link
Contributor

@rnett rnett commented Apr 29, 2021

Uses the spotless plugin for formatting. Will check formatting in the initialize phase (so its before codegen & native builds), and auto-format can be ran with mvn spotless:apply. We can use it for the Kotlin API as well once diffplug/spotless#142 is fixed.

I have it set to ratchet from master (i.e. only check changed files), do we want to do a big "apply formatting everywhere" PR (it's ~500 files)?

Signed-off-by: Ryan Nett <JNett96@gmail.com>
Signed-off-by: Ryan Nett <JNett96@gmail.com>
Signed-off-by: Ryan Nett <JNett96@gmail.com>
@rnett rnett mentioned this pull request May 18, 2021
@karllessard
Copy link
Collaborator

I'm not convinced about reformatting source code with Maven, any reason why simply using Google Java Style formatting isn't enough?

@rnett
Copy link
Contributor Author

rnett commented May 21, 2021

The XML differs a bit from the actual plugin, at least for me (things like line length and single line comments). Plus, it's easy to forget and have a bunch of improperly formatted files end up in the repo, like we have now. This doesn't actually do any formatting, it just checks it as part of the build, like the lint checks.

@karllessard karllessard merged commit ceae489 into tensorflow:master May 23, 2021
@karllessard
Copy link
Collaborator

@rnett I should have labeled that PR with CI build before merging... can you please check the multiple errors we are facing on many platforms from GitHub Actions? I might need
to revert that PR unless you find a quick fix for it, thanks

@rnett
Copy link
Contributor Author

rnett commented May 23, 2021

Ugh, it seems to be that --add-exports isn't recognized for JDK 8, but is required for 16. I'll remove and ignore that file for now.

@rnett
Copy link
Contributor Author

rnett commented May 23, 2021

Do you or @Craigacp know if there's a way to have a version-dependent .mvn/jvm.config file?

@rnett
Copy link
Contributor Author

rnett commented May 23, 2021

See #324.

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.

None yet

2 participants