SpotlessCache extension for Eclipse formatters #279
Conversation
Provided proposal for SpotlessCache extension required by some Eclipse Formatters.
|
@nedtwigg I will continue with the CDT integration on a separate branch. The CDT is quite independent and as mature as JDT. So if you like, we can either merge this branch, or continue the development until the integration of WTP in lib/lib-extra is complete. |
|
Sorry for slowness, I'll take a look this weekend. |
|
|
||
| private enum WTP { | ||
| CSS("body {\na: v; b: \nv;\n} \n", "body {\n\ta: v;\n\tb: v;\n}", WtpEclipseFormatterStep::createCssBuilder), HTML("<!DOCTYPE html> <html>\t<head> <meta charset=\"UTF-8\"></head>\n</html> ", "<!DOCTYPE html>\n<html>\n<head>\n<meta charset=\"UTF-8\">\n</head>\n</html>\n", WtpEclipseFormatterStep::createHtmlBuilder), JS("function f( ) {\na.b(1,\n2);}", "function f() {\n a.b(1, 2);\n}", WtpEclipseFormatterStep::createJsBuilder), JSON("{\"a\": \"b\", \"c\": { \"d\": \"e\",\"f\": \"g\"}}", "{\n\t\"a\": \"b\",\n\t\"c\": {\n\t\t\"d\": \"e\",\n\t\t\"f\": \"g\"\n\t}\n}", WtpEclipseFormatterStep::createJsonBuilder), XML("<a><b> c</b></a>", "<a>\n\t<b> c</b>\n</a>", WtpEclipseFormatterStep::createXmlBuilder); | ||
|
|
There was a problem hiding this comment.
This can be more readable with:
// @formatter:off
private enum WTP {
CSS("body {\na: v; b: ...
HTML("<!DOCTYPE html> ..
}
// @formatter:on| /** Provides default configuration for CSSformatter */ | ||
| public static EclipseBasedStepBuilder createCssBuilder(Provisioner provisioner) { | ||
| return new EclipseBasedStepBuilder(NAME, " - css", provisioner, state -> apply("EclipseCssFormatterStepImpl", state)); | ||
| } |
There was a problem hiding this comment.
I might be missing something, but I think you can avoid adding the formatterStepExt variable to EclipseBasedStepBuilder and EclipseBasedStepBuilder.State if you apply the following change:
- new EclipseBasedStepBuilder(NAME, " - css", ...
+ new EclipseBasedStepBuilder(NAME + " - css", ...There was a problem hiding this comment.
Currently I use formatterName to lookup the dependencies, which are equal for the different format methods (JSONS, JS, HTML...). So question remains whether I need a different name.
Each format method name per extension must be unique. Actually I should have not made this change, but addressed the question instead, whether we want a distinct FormatterExtension.
For typescript-formatter, there was the decision not to have a separate formatter extension.
But I understood that the typescript-formatter is one formatter for all. This is not the case for WTP. I can use the default WTP file extension settings to pick a formatter method for a file, but that should be configurable... So maybe it is worth to have dedicated FormatterExtension.
There was a problem hiding this comment.
Currently I use formatterName to lookup the dependencies
Aha! That's what I was missing. In that case, this LGTM!
There was a problem hiding this comment.
Re: FormatterExtension, you means a typescript {} extension and a javascript {} extension (and css {} and html {})? I lean towards the default FormatterExtension, but I'm okay with having separate extensions instead. Here is my argument:
"Convention-over-configuration" is a great idea, and gradle has strong conventions for all the JVM languages. For the JVM languages we support (java{}, groovy{}, kotlin{}, scala{}), we can slurp the file locations from gradle's project model, and take advantage of that convention. In my gradle web projects, I've found that there aren't strong conventions for the location of web resources. Sometimes they're kinda scattered around in the src/main/resources directory, but other times they're being used for a static site outside of the JVM context, and they're just in the project root. So we're not really able to use the "convention-over-configuration" to simplify setup for web resources.
On the other hand, it probably helps user-discoverability to have dedicated html {}, css {}, etc. blocks... The real proof will be the docs. If it's easier to document html {}, css {} - dedicated formats, then that's what we should do.
We already have a sql{} block that isn't able to use conventions to know where the sql files are...
There was a problem hiding this comment.
OK, I'll remove the name extension and aim for one FormatExtension per WTP formatter method.
|
Sorry, @nedtwigg, I think I found a mistake in my approach: The Is my understanding correct? If my understanding is correct: |
Yes.
Since we are both spotless devs, I think we overestimate how much people change settings. I think 99+% of the calls to That said, if you see an easy way to make it more resource-efficient, I'm all for it :) |
|
I have to call it a day. Got too late... Will have time end of the week to make up my mind. |
|
@nedtwigg Had a look at the memory development of the |
|
Then I think all this needs is to update the changelogs and merge :) Press the merge button whenever you're ready. |
|
@nedtwigg See no need for change-log entry, do you? The cache modification is quite low level and the WTP integration has not finished. |
|
Good call. |
Started WTP integration.
Provided proposal for SpotlessCache extension required by some Eclipse Formatters.
The extension shall support the following scenarios:
Both scenarios are covered by the new tests. The WTP is basically just an example, but the workspace level configuration and usage of statics is quite common.
The solution itself is just a proposal. Feel free to make changes on this branch.