Adds a different XML formatter #294
Conversation
There was a problem hiding this comment.
I like the idea of using Eclipse's XML formatter... but I wonder if we can add an external dependency rather than copy/paste the formatting code into this project. The scope of this project is to execute other formatter libraries on a Maven project, not to be a formatter library itself.
| @@ -451,6 +451,7 @@ | |||
| <exclude>LICENSE</exclude> | |||
| <exclude>NOTICE</exclude> | |||
| <exclude>.gitattributes</exclude> | |||
| <exclude>**/xml/eclipse/*.java</exclude> | |||
There was a problem hiding this comment.
Rather than suppress the header applied to this project, please contribute your code under the project's Apache 2.0 license, and add any copyright notices to the NOTICE file, if you wish to do so.
If you wish to also make your code available under additional licenses, you're obviously free to do so, but please conform to the project's documentation standards when contributing here. A class-level comment (below the license header), for example, could serve as a notice to users that the code is also available under the EPL.
There was a problem hiding this comment.
Class level comment sounds good to me, I could add the attribution to original authors there and allow mycila to write the Apache header 👍
| * File or classpath location of a properties file to use in eclipse xml formatting. | ||
| */ | ||
| @Parameter(defaultValue = "formatter-maven-plugin/eclipse/xml.properties", property = "configexmlfile", required = true) | ||
| private String configEXmlFile; |
There was a problem hiding this comment.
Using the Eclipse XML formatter makes a lot more sense to me, since this project's main purpose is to automate the application of Eclipse formatting rules. I don't know much about jsoup, but I've yet to find its formatting useful.
Rather than create a new formatter option like this, I can think of two possible alternatives:
- This replaces the jsoup implementation
- We do something like what I proposed in Create separate mojos/goals for separate formatters #254 to create separate mojos for each formatter, rather than overload the one
There was a problem hiding this comment.
Yes, I think Eclipse's formatting is much more useful thatn jsoup's, I only left it as default in order to prvent breaking existing users. So if you're ok with a replacement I'm 100% good with that.
There was a problem hiding this comment.
Totally agree Jsoup solution was never actually useful. What was really wanted was what we could do in Eclipse directly and that was a 'beta' type attempt to get something that eventually might evolve so if we have a better solution, jsoup certain can go. I would highly doubt anyone is actually using the jsoup solution provided here as it is almost entirely not readable at a human level how we would expect things. So I don't think it even warrants deprecation but straight replacement.
There was a problem hiding this comment.
OK. If we resolve the other issues raised I'll change the PR to replace jsoup entirely.
src/main/resources/formatter-maven-plugin/eclipse/xml.properties
Outdated
Show resolved
Hide resolved
| * Based on Eclipse Ant Formatter and distributed under original Eclipse 2.0 license and compatible Apache 2.0 See: | ||
| * https://github.com/eclipse/eclipse.platform/tree/master/ant/org.eclipse.ant.ui/Ant%20Editor/org/eclipse/ant/internal/ui/editor/formatter | ||
| */ | ||
| public class XmlDocumentFormatter { |
There was a problem hiding this comment.
This file (and others) are going to get dramatically reformatted on this project's first build.
|
|
||
| /** | ||
| * Based on Eclipse Ant Formatter and distributed under original Eclipse 2.0 license and compatible Apache 2.0 See: | ||
| * https://github.com/eclipse/eclipse.platform/tree/master/ant/org.eclipse.ant.ui/Ant%20Editor/org/eclipse/ant/internal/ui/editor/formatter |
There was a problem hiding this comment.
If this code is already available in a separate library, can that library simply be pushed to Maven Central and reused? I'm not a fan of copying external code in when a dependency will do.
|
@jam01 Jsoup solution here was never actually viable due to formatting issues as it was better at a parsing solution rather than human readable so I would simply replace the code. However, I do have some reservations on copying chunks of code that are licensed from elsewhere and dated rather old on top of it. Can you elaberate on what is effectively the original work of art here vs what was ported and what if any inability to use the original work as a separate module simply extended like we do with other libraries? |
|
I initially tried to get Eclipse's default XML formatter working directly as a dependency, I'll get to that in a sec. As far as the formatting implementation code in this PR, it is a moderately modified version of the Ant formatter code. It makes some changes as suggested by the modernizer plugin as well as adding the multi lined attributes feature. Because the original code is licensed under Eclipse 2.0 I only added Apache 2.0 as an additional license in order to retain the original attributions. My understanding of these two licenses tells me this is completely acceptable, perhaps you guys know more. Ok so I ended up actually creating the equivalent of the jsdt-core dependency that java-formatter uses but ultimately I could not get it to work. There are some issue with missing global preferences and bundles and bundle contexts that I just could not resolve. It seems to me it requires a fair amount of things that will only be available when the platform is running. So I decided to try to port the Ant version since it had much less coupling to Eclipse itself (Ant XML formatter is different from default XML formatter). It ended up working out and this PR is the result. |
|
Otherwise I think if we get the licensing right for this modified implementation, it would still be a great addition. |
|
I'm not a lawyer, but my understanding is that you cannot take EPL licensed code and relicense it under Apache (unless you own the copyright for that code, and then you can release it under any license you want). I believe the EPL code, modified or unmodified, needs to stay EPL, and mixing it in like you did would create a derivative work, which would need to be EPL, and abide by its reciprocal conditions... which is counter to this project's own license (Apache 2.0), which is much less restrictive. I think the best option is to create a separate self-contained library, under the EPL license, that contains all the EPL-licensed formatting code you want (without attempting to relicense or dual license it), and then we can add it as a dependency to this project, and leverage it instead of jsoup. If you want to do that work, we could host it under the revelc group, next to this project. As long as we're just executing that library's code, and not integrating it into this project, I think we're fine from a licensing point of view... and it also works better from the perspective of keeping the scope of this project narrow. |
|
I went through the license agreement and other documentation and my understanding was that though it is certainly derivative work it was possible to add a secondary license which allows it to be used in the context of that second license. https://www.eclipse.org/legal/epl-2.0/faq.php talks about this specifically. Ultimately I'm not a lawyer either and completely understand if you don't feel comfortable adopting the code. I suppose I'll be withdrawing the PR. |
|
The instructions on that FAQ appear to me to simply be instructions on how to document that something is dual-licensed as Apache 2.0 as well as EPL 2.0, if the copyright holder were to choose to do so. But, you'd still have to be the original copyright holder to have the right to dual license it... the EPL 2.0 doesn't grant that. It does grant something called a "Secondary License", but that is a special case that only applies to releasing within a larger GPL work (GPL, specifically, not others). Regardless, I think this was a useful discussion. I certainly learned a lot, since I have never really looked at EPL before. I think there's merit to this feature being added... I just think it needs to be reworked as a separate jar first. I'm not comfortable adding it as is (because of both license issues, as well as it causing scope creep / complexity that is best addressed as a separate module). If there's anything I can do to help rework it as a separate project, please let me know. If you want, we could host it as another revelc repo... perhaps https://github.com/revelc/xml-formatter (or similar)? Or you could host it on your own GitHub and publish to Maven Central on your own, and then we can use it here. Whatever works for you. For now, I'm closing this issue, since I don't think we should merge as-is, but let's keep the conversation going if you need help reworking it to be added in a different way. |
|
I definitely appreciate the offer and time to work through PR. I'll give another shot at getting the default formatter working and let you know what happens. To make sure I understand the alternative of packaging this as a separate revelc library... You're suggesting we could make a standalone project out of the formatter implementation, license it under EPL 2 as required, and distribute it to maven central either under my name or revelc. That'd allow the formatter plugin to make use of the library. Did I get that right? |
|
@jam01 Yes, that is exactly what I was suggesting. 😺 |
|
Here we go https://github.com/jam01/xml-formatter I tried to transfer the repo into revelc, I thought by requesting to transfer the repo it'd ask you guys for approval but it seems I need permissions on revelc which I obviously don't have. You can just clone and upload, or let me know how you want to do this. |
|
I accepted the transfer and moved it to revelc. |
|
Great! I'll need some guidance on the versioning and publishing before submitting a PR for use with the maven plugin. |
Adding a different formatter based on Eclipse's Ant formatter. Couple of features compared to jsoup are that it keeps line breaks, wraps attributes after a specified length, and supports breaking each attribute into its own line. There's probably a couple of more tests missing and perhaps doing some sanity checks on the xml before formatting, but I wanted to check the general idea and contribution was good.