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

Spaces in self-closing tags #75

Closed
AndyFWealthfront opened this issue Jun 30, 2020 · 11 comments
Closed

Spaces in self-closing tags #75

AndyFWealthfront opened this issue Jun 30, 2020 · 11 comments

Comments

@AndyFWealthfront
Copy link

I notice that sortpom will insert spaces in self-closing empty tags, <likeThis />. Self-closing tags are are not common in Maven but we have a few in some plugin configurations, like <requireUpperBoundDeps />.

While I don't feel strongly either way about this format, it's different than IntelliJ's default and I was curious if this was a conscious choice. It causes some noise for us when developers use IntelliJ's auto-format and then our build system reverts it with sortpom, potentially complaining about unexpected working tree changes. We have lots of non-pom XML files already formatted <withNoSpace/> so I am reluctant to change the IntelliJ XML format settings.

I didn't see a way to configure this behavior in sortpom. Would you consider adding the ability to configure this behavior to use no space instead? (Or did I miss it and it's already supported?)

Another alternative, if you don't want to add a new parameter, would be for sortpom to allow either one or zero spaces to remain unchanged. Then at least it wouldn't fight with IntelliJ, even if the pom might end up with a mixed format.

@Ekryd
Copy link
Owner

Ekryd commented Jun 30, 2020

This sounds interesting. I will have a look and see what I can do.

@Ekryd
Copy link
Owner

Ekryd commented Jun 30, 2020

I think that this problem can be solved. A parameter 'spaceBeforeCloseEmptyElement' (default true) can be added. Just set this parameter to false in your configuration to remove the preceding space. I have to change some internal handling in the plugin and create tests before I create a snapshot release.
After that I need you to test the plugin before I can make a new release.

@AndyFWealthfront
Copy link
Author

Great! I would be happy to test it when it's ready.

@Ekryd
Copy link
Owner

Ekryd commented Jul 1, 2020

Time for testing!

  1. Clone the repository
  2. Build with mvn clean install
  3. Use the plugin with mvn com.github.ekryd.sortpom:sortpom-maven-plugin:2.11.1-SNAPSHOT:sort and remember to use the new parameter -Dsort.spaceBeforeCloseEmptyElement=false or false

Let me know how it works for you

@AndyFWealthfront
Copy link
Author

Works great!

I tried it with the command-line -Dsort.spaceBeforeCloseEmptyElement=false, as well as <properties><sort.spaceBeforeCloseEmptyElement>false</sort.spaceBeforeCloseEmptyElement></properties>, and the plugin configuration element <spaceBeforeCloseEmptyElement>false</spaceBeforeCloseEmptyElement>, and I flipped back and forth between true and false. Everything worked as expected.

<spaceBeforeCloseEmptyElement> took precedence over the command-line flag when both were provided with opposite values, which surprised me a little, but if that's expected then it's certainly not a problem. Thanks for such a quick turnaround.

@Ekryd
Copy link
Owner

Ekryd commented Jul 1, 2020

No problem, you picked a good spot for a feature request :-)

I agree with you that it is strange that the configuration takes precedence. I think it is hardcoded in Maven (let me know otherwise). Thanks for your thorough testing, I will try to release it tomorrow.

Oh! And by the way, have you tried the 'verify' goal in SortPom? It ignores any formatting and only reorders when elements need to be reordered. I do recommend using this for build systems.
The build system uses sortpom:verify and is configured to fail the build if the elements have the incorrect order. The developers responsibility is then to fix the ordering (probably with sortpom:sort) and make another commit. It does make some room for special cases in some pom-files (if needed).

@AndyFWealthfront
Copy link
Author

Thanks for the suggestion. We have bound sortpom:sort to the validate phase with createBackupFile=false, which doesn't guarantee that the pom will always be sorted, but developers are running mvn validate often enough that the sorted pom gets committed pretty soon if anyone introduces some incorrect formatting. This doesn't leave room for special cases but we haven't found that to be a problem.

I find that this mostly achieves the goal of "all our poms look the same" without causing any headaches for developers who already have to deal with picky build failures from the checkstyle and enforcer plugins.

If I decided that the poms were still not being sorted often enough, rather than push it back on the devs with a build failure, I'd probably add a step to our build system to automatically commit the sortpom changes to the side branch! How's that for an opinionated linter :-) "Oh I see you did it wrong, here let me change that for you, you're welcome!"

@Ekryd
Copy link
Owner

Ekryd commented Jul 2, 2020

Sonatype have changed the way to deploy artifacts. I may need to change the deployment plugin, which will cause a delay.

@Ekryd
Copy link
Owner

Ekryd commented Jul 2, 2020

Hi, SortPom version 2.12.0 is now released with the spaceBeforeCloseEmptyElement parameter. I hope that you enjoy it!

@AndyFWealthfront
Copy link
Author

Thanks again. I've already updated to this version in my company's parent pom and set the flag :-)

@Ekryd Ekryd closed this as completed Jul 3, 2020
@blacelle
Copy link

blacelle commented Feb 5, 2023

The default has been changed from true to false with sortPom3.0.0 https://github.com/Ekryd/sortpom/wiki/Parameters.

One can note maven-release-plugin behave like with -Dsort.spaceBeforeCloseEmptyElement=true, (e.g. solven-eu/pepper@1436c98#diff-b5a06276719e759fe07dfe6f75d781be5f83d2215179d82bdb195ad035348214R251)

One may trigger sortPom during the release process with https://maven.apache.org/maven-release/maven-release-plugin/prepare-mojo.html#preparationGoals

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

No branches or pull requests

3 participants