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

Change formatter plugin to @diffplug's Spotless plugin #198

Closed
wants to merge 13 commits into from

Conversation

jbduncan
Copy link
Contributor

@jbduncan jbduncan commented Feb 18, 2018

Mandatory disclaimer: I am a contributor to Spotless, so I'm naturally biased towards it, but despite this please allow me to explain why I think we should adopt this plugin over the plugin we currently use. :)

  1. Error messages are (in my opinion) easier to understand.
    For example, we currently have messages like
Found 1 non-complying files, failing build (validateOnly is true)

which aren't terribly informative - they don't explain which files no longer comply and why, they don't explain how to fix the problem, and the messages easily get lost in the noise of other Maven logging messages.
But with Spotless, we'd have messages like

The following files had format violations:
src\main\java\edu\uci\ics\jung\graph\DelegateCTree.java
@@ -203,8 +203,7 @@
··@Override
··public·boolean·removeNode(N·node)·{
····checkNotNull(node,·"node");
-····if·(!nodes()
-········.contains(node))·{
+····if·(!nodes().contains(node))·{
······return·false;
····}
····for·(N·nodeToRemove·:·Graphs.reachableNodes(delegate,·node))·{
Run 'mvn spotless:apply' to fix these violations.

which AFAICT fix the problems above rather nicely.

  1. We can easily adopt formatters other than google-java-format in the future, such as a formatter which checks that files have license headers that follow a certain template. (This seems useful or even important for us, since currently the license headers in JUNG's source files are a bit inconsistent.)

  2. If Spotless's Maven plugin ever exposes a "padded cell" option in the future (currently only Spotless's Gradle plugin exposes such an option), and if we ever discover that google-java-format or another formatter has bugs which prevent it from producing consistent results (idempotency bugs), then we can use "padded cell" in the short-term to ensure that that formatter continues producing consistent results until the respective bug is fixed.

Advantages of Spotless over current solution:
- Error messages are (in my opinion) easier to understand.
- We can easily adopt other formatters in the future, such as a
  formatter which checks that files have license headers that follow
  a certain template.
…CI" commits

This reverts commit f2dfcff.

It seems that everything would need to be changed from CRLF endings to LF endings,
which seems unfeasible and strange.
These were accidentally introduced when fixing a previous merge
conflict.
This reverts commit 2eab4c0.

Apparently changing to an earlier Spotless version that I thought
worked previously, doesn't fix the problem! It might be that the
problem was masked before I added commit jrtom@3c06a77
@jbduncan
Copy link
Contributor Author

@jrtom Hmm, I'm having trouble getting Travis CI to pass now. It seems there's a problem on the Spotless side (to do with unexpected CRLF line endings) which has left me stumped. So when I next have free time, I'll probably elect to close this PR and create a new one, to see if starting things afresh fixes things.

@jrtom
Copy link
Owner

jrtom commented Jul 24, 2018

I'm happy to review this (or a subsequent CL) whenever you have time to sort out what's going on.

@jbduncan
Copy link
Contributor Author

Whoops, I obviously forgot about this PR! I'll put it at the top of my mental open-source-things-to-tackle list. :)

@jrtom
Copy link
Owner

jrtom commented Jul 24, 2018

No worries, no rush, I was just reminded of it by your comment on the Spotless issue. :)

@jbduncan
Copy link
Contributor Author

I'm about to push a new PR to see if that fixes things, so I'm closing this PR.

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