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 Autostyle for formatting #12

Closed
wants to merge 1 commit into from
Closed

Use Autostyle for formatting #12

wants to merge 1 commit into from

Conversation

vlsi
Copy link
Contributor

@vlsi vlsi commented Jan 12, 2020

Copy link
Contributor

@szpak szpak left a comment

Choose a reason for hiding this comment

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

I don't know the community(s) related to those plugins, but to do not generate more merge conflicts I would like to wait with potential merge of that PR until #9 is merged.

@marcphilipp
Copy link
Member

@vlsi What was the reason for forking Spotless?

@vlsi
Copy link
Contributor Author

vlsi commented Jan 12, 2020

Contributing to Spotless is hard :(
It seems to take more time than maintaining a fork.

I did try to contribute a feature: diffplug/spotless#485
Unfortunately, the PR feedback looked like a bikeshedding to me :(

Spotless is written in Java, and their choice for multi-line strings in tests is

		assertTaskFailure(task,
				"    testFile",
				"        @@ -2,9 +2,9 @@",
				"         u 1",
				"         u 2",
				"         u 3",

which is very unpleasant to edit and update :(

The PR was merged, however, then I found a bug in my implementation:

Vladimir: current.getEndA() - next.getBeginA() should flipped like next.getBeginA() - current.getEndA()

Ned has chosen to revert the PR altogether.

Just in case: the bug was not detected by Spotless test suite since none of the tests included a case when a file had multiple violations that were separated with more than 3 lines.


Ned is nice, however, their priorities seem to be tangential to my needs.

For instance, Spotless does not support license headers for kts, xml, sh files, it does not support package-info.java, it often produces "stacktrace-like" messages (e.g. unable to parse ...) where a single pointer to the line and position were enough.

For instance, there's an open issue diffplug/spotless#496.
As you can see the proposed solution there is to add $FIRST_LINE to the license header.

On the other hand, I reworked that in Autostyle, so the license text is declared just once, and Autostyle formats it using the appropriate comment styles (e.g. it knows Java-like, XML, Shell, Windows CMD files).


Spotless uses obscure Gradle techniques, so you can't re-execute the task that failed (you can't really copy-paste the task name from the failure log).
It uses obscure techniques to configure the fileset, and it does not really support multi-project layouts (see diffplug/spotless#468)
It used to produce Gradle compatibility warnings, and it seems still rely on afterEvaluate which is sad for multi-module projects.

It does not support Gradle Build Cache.


Have you seen "paddedcell warning" in Spotless?
It happens in case there's a runaway rule (e.g. a rule that adds a space to the end of the line on each execution).

When that happens Spotless provides no output on the actual contents, so you can't really understand what is misbehaving.


For instance, I've migrated Apache Calcite from Spotless to Autostyle: apache/calcite#1682, Apache Calcite Avatica: apache/calcite-avatica#118, Apache JMeter: apache/jmeter#549.

It increased the consistency of the source code, and it improved the error messages.


All of the above (+ migration to Kotlin + migration to JUnit 5) seemed to be easier to implement in Autostyle than discussing in Spotless issues :(

@marcphilipp
Copy link
Member

@vlsi Thanks for the detailed explanation! I'll take a closer look.

@marcphilipp
Copy link
Member

@vlsi No offence, I think we'll stay with Spotless for now which works well enough for what we need for this small project here.

@marcphilipp marcphilipp closed this Feb 6, 2021
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

3 participants