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

Spotless formatting #2524

Closed
wants to merge 16 commits into from
Closed

Conversation

MaicolAntali
Copy link
Contributor

Purpose

Adds the Spotless plugin to check that formatting follows the Google Java Style Guide

Description

Related with #2284

⚠️ This is a daft PR to discuss the configuration and how run it (in a new workflow or use the build workflow).
(I have applied some default configurations)

Checklist

  • New code follows the Google Java Style Guide
  • If necessary, new public API validates arguments, for example rejects null
  • New public API has Javadoc
    • Javadoc uses @since $next-version$
      ($next-version$ is a special placeholder which is automatically replaced during release)
  • If necessary, new unit tests have been added
    • Assertions in unit tests use Truth, see existing tests
    • No JUnit 3 features are used (such as extending class TestCase)
    • If this pull request fixes a bug, a new test was added for a situation which failed previously and is now fixed
  • mvn clean verify javadoc:jar passes without errors

Copy link
Member

@eamonnmcmanus eamonnmcmanus left a comment

Choose a reason for hiding this comment

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

Thanks, this is very helpful! Just a few comments. If mvn verify passes then I think this should be good.

pom.xml Outdated Show resolved Hide resolved
pom.xml Outdated Show resolved Hide resolved
pom.xml Outdated Show resolved Hide resolved
@MaicolAntali
Copy link
Contributor Author

If mvn verify passes then I think this should be good.

I thought the same thing, also because the documentation says:

By default, spotless: check is bound to verify maven phase.

Until, I executed:mvn spotless:check and some error came out. I don’t know why they don’t come out with mvn check 🤔

@eamonnmcmanus
Copy link
Member

Until, I executed:mvn spotless:check and some error came out. I don’t know why they don’t come out with mvn check 🤔

Well, can you temporarily introduce a deliberate formatting error into the PR, and verify that the CI fails?

Moves the `spotless` plugin out of `<pluginManagement>`
@MaicolAntali
Copy link
Contributor Author

For the first time, I'm happy that CI fails 🤣

I will run mvn spotless:apply, check that every changes is correct and then I'll commit the changes.

pom.xml Outdated Show resolved Hide resolved
Comment on lines +145 to +158
<formats>
<format>
<includes>
<include>*.md</include>
<include>.gitignore</include>
</includes>
<trimTrailingWhitespace/>
<endWithNewline/>
<indent>
<spaces>true</spaces>
<spacesPerTab>4</spacesPerTab>
</indent>
</format>
</formats>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this <formats> section needed? If I understand it correctly that would only apply to *.md and .gitignore the way it is currently configured; and I am not sure if we should impose any formatting rules on them.

Copy link
Contributor Author

@MaicolAntali MaicolAntali Nov 6, 2023

Choose a reason for hiding this comment

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

Formatting rules are very weak on.md and .gitignore. They replace tab characters with 4 spaces and trim trailing whitespace.

I think they are good general rules, but if you prefer to remove them, let me know.

Copy link
Collaborator

@Marcono1234 Marcono1234 Nov 6, 2023

Choose a reason for hiding this comment

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

Ok, I just asked because this section looked like the section from the Spotless examples and it isn't needed for Java formatting.

The combination of *.md with trimTrailingWhitespace should be removed though because they can break Markdown formatting. Arguably the Spotless examples are also flawed (diffplug/spotless#1877).

Edit: Nevermind, if the Markdown files are consistently using a trailing \ instead of two spaces for a hard line break, then it should be fine to remove trailing whitespace. But it might be good to add a comment here mentioning that a trailing \ can be used instead of trailing whitespace in Markdown files.

*
* @since 1.8
*/
public static final JsonNull INSTANCE = new JsonNull();

Check notice

Code scanning / CodeQL

Deprecated method or constructor invocation Note

Invoking
JsonNull.JsonNull
should be avoided because it has been deprecated.
gson/src/main/java/com/google/gson/Gson.java Fixed Show fixed Hide fixed
@MaicolAntali
Copy link
Contributor Author

I'm not sure why the Build fails.
If I run mvn clean verify javadoc:jar on my machine, everything works fine

@eamonnmcmanus
Copy link
Member

It's great to see this progress!

A couple of things. First, I think reformatting the existing files should be a separate PR from the one that adds the Spotless plugin. Second, I'm pretty sure the CI failure is because Java17RecordTest uses Java record, and in the JDK 11 configuration that doesn't work. You could reasonably exclude that one file from formatting.

@MaicolAntali
Copy link
Contributor Author

Make sense, I reverted the formatting commits and I will move it in a new PR (when this PR will be merged, so I can edit the pom.xml to exclude Java17RecordTest)

@MaicolAntali MaicolAntali marked this pull request as ready for review November 6, 2023 17:12
@eamonnmcmanus
Copy link
Member

Make sense, I reverted the formatting commits and I will move it in a new PR (when this PR will be merged, so I can edit the pom.xml to exclude Java17RecordTest)

I think you need to merge that PR first, so that you won't get errors from this one?

@MaicolAntali MaicolAntali marked this pull request as draft November 6, 2023 19:37
@MaicolAntali
Copy link
Contributor Author

MaicolAntali commented Nov 6, 2023

All CI passed, but I think is better have a more tidy commits history.
Tomorrow:

  • I will move edde156 commit in a single PR
  • I'll create a new PR to apply the spotless plugin.

And I'll close this PR.
I hope that will help to obtain a more tidy and clear commits history

@Marcono1234
Copy link
Collaborator

@eamonnmcmanus if possible please wait a bit with merging this (or any related formatting PR). #2531 'broke' some formatting of Markdown files which intentionally used two trailing spaces for hard line breaks, and in theory someone would have to review all the 229 files changed by the last PR (in case you haven't done so already).

I can try to revert the Markdown formatting changes (and possibly any other unintentional changes), but only in the next days.

@eamonnmcmanus
Copy link
Member

I can try to revert the Markdown formatting changes (and possibly any other unintentional changes), but only in the next days.

I can do that. I will refrain from sharing my thoughts on that particular Markdown syntax, except to say that the alternative syntax with \ seems preferable.

@Marcono1234
Copy link
Collaborator

I can do that. I will refrain from sharing my thoughts on that particular Markdown syntax, except to say that the alternative syntax with \ seems preferable.

Thanks! I completely overlooked in the specification that a trailing \ achieves that as well; that is certainly better than the invisible trailing spaces.

Sorry that my previous comment was not that kind. I think it is great though that you both are working on making the formatting of this project consistent!

@MaicolAntali
Copy link
Contributor Author

This PR is now replaced by #2536 and #2537

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.

3 participants