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

Provision of Spotless Eclipse XML formatter plugin. #230

Closed
wants to merge 4 commits into from

Conversation

fvgh
Copy link
Member

@fvgh fvgh commented Apr 4, 2018

XML formatter plugin based on Eclipse WTP-WST for #140.

@fvgh fvgh requested a review from nedtwigg April 4, 2018 12:22
@fvgh
Copy link
Member Author

fvgh commented Apr 4, 2018

The positive aspects are:

On the downside, we would need an interface extension, since the XSD/DTD location might be relative to the file which shall be formatted.
The approach uses currently a fat JAR, which takes 5.4 MB. I would like to stick to the solution for now and refactor it as part of #226.

Please have a look whether you can agree first of all to the interface extension. In that case I can continue to work on a proposal for lib-extra

@fvgh fvgh mentioned this pull request Apr 5, 2018
@nedtwigg
Copy link
Member

nedtwigg commented Apr 5, 2018

Wow! I don't know much about XML, but this looks really powerful! Lots of know-how went into mocking out the eclipse bundle system, great work!

It seems that all the links just go to the top of the diff. I looked at all the files, but I'm not sure what you're referencing re: interface extension. As far as publishing and releasing this as a fat jar, I give it a thumbs-up, once I understand the interface extension idea.

It looks like the biggest remaining challenge will be the user-interface and documentation. Since this is all in _ext, I think we should merge this now, and then we can handle the UI and docs tasks in a smaller PR.

@fvgh
Copy link
Member Author

fvgh commented Apr 5, 2018

The normal interface is:
public String format(String raw)
XML needs the following, since it needs to know the file path it is modifying for XSD/DTD/XInclude lookup:
public String format(String raw, String baseLocation)

This violates the simple/efficient concept of Spotless formatters where you just build up a chain of String processors.
Sure that you can agree to the extension?

Possible work-around would be to analyze all XML files first and generate an "internal catalog", but that's quite error prone...

Before merge, we should discuss whether I should instead of providing a XML formatter plugin, I should provide a WDT plugin, that supports also JS/HTML formatting. I made a corresponding proposal for #162 .

@nedtwigg
Copy link
Member

nedtwigg commented Apr 5, 2018

If an XSD/DTD/XInclude file changes, can that change the formatting of another file? If so, then either:

A) these XSD/DTD/XInclude files need to be captured as config files in the step's state anyway. we do something on this scale with .gitattributes files already.
B) the XML step will not support up-to-date checking

I'm fine with either outcome. We won't have to change interfaces, because the raw file is already available (originally intended for skipping package-info.java).

/**
* Returns a formatted version of the given content.
*
* @param rawUnix
* the content to format, guaranteed to have unix-style newlines ('\n'); never null
* @param file
* the file which `rawUnix` was obtained from; never null. Pass an empty file using
* `new File("")` if and only if no file is actually associated with `rawUnix`
* @return the formatted content, guaranteed to only have unix-style newlines; may return null
* if the formatter step doesn't have any changes to make
* @throws Exception if the formatter step experiences a problem
*/
public @Nullable String format(String rawUnix, File file) throws Exception;

@fvgh
Copy link
Member Author

fvgh commented Apr 5, 2018

Glad to here that it has not that much code impact.

About the "up-to-data" check:
Yes, as you can see in the examples, the DTD/XSD configures the XML format. This is explicitly foreseen by the W3C and had originally been introduced for XLST, but is also respected by WDT formatter.
You are right about the up-to-date check, but wrong about it's correspondence to the "internal catalog".
It is quite common that XSDs for example include other XSDs and furthermore not all of them need to be part of the work-space (can be referenced by http, but also be within JARs). Only possibility to detect all involved external documents, is an analysis of the WDT/CM document cache after initial run. But since WDT just ignores XSD/DTD/..., which it cannot resolve, that result might be erroneous.

To conclude:
I would not add any up-to-data checks. The user should know about the --rerun-tasks if he/she does fancy things.

As stated in previous comment, the provision of a WDT plugin instead of the XML plugin might be useful for the further development of spotless. Any thoughts on this one?

@nedtwigg
Copy link
Member

nedtwigg commented Apr 5, 2018

Sounds good. If we just do implements FormatterStep then the param you want will be there, and it won't do any up-to-date checking.

Re: WDT, if you want to use WDT, then I want to add it. If you're only adding WDT in case other people might want it, then I think we should leave it out. It's easy to build things, but it's hard to be sure that they're useful unless you're actually using them on real projects with real people.

@fvgh
Copy link
Member Author

fvgh commented Apr 5, 2018

WDT I use, but I only use Core/XML/XSD/XSD.
It also provides HTML, which requires JS and all modules mentioned above.
So if I include HTML (and therefore JS), this solution would provide a solution for #162 and #140.
Unfortunately I found that solving #191 would need some of the extensions I provided here. Do you see the possibility to split the Eclipse Mocking off the WDT and make it a separate lib on Maven Central?

@nedtwigg
Copy link
Member

nedtwigg commented Apr 5, 2018

I think there's probably a pretty good hunk of demand for your eclipse mocking as its own library! Happy for it to live and publish from _ext for however long you would like, but I bet it could find new users of its own for other applications as fvgh/eclipsemock too :)

@fvgh fvgh closed this Apr 5, 2018
@fvgh
Copy link
Member Author

fvgh commented Apr 5, 2018

The branch will be split into eclipse-base (containing Spotless OSIG bundle controller and Eclipse services) and eclipse-wdt (extension of current eclipse-xml also supporting WDT formatter for JS and HTML).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants