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

Revisit schema location indent choice #413

Closed
mches opened this issue Mar 31, 2024 · 11 comments · Fixed by #414
Closed

Revisit schema location indent choice #413

mches opened this issue Mar 31, 2024 · 11 comments · Fixed by #414
Labels
enhancement released This is now part of the plugin

Comments

@mches
Copy link
Contributor

mches commented Mar 31, 2024

First, allow me to say thank you for SortPom.

The conscious choice of 2 * indent + 1 space as a continuation indent seems to me an unusual one. In practice, I've only ever seen 2 * indent or 1 * indent continuation indents, no matter whether the indents are spaces or tabs. For that reason, how do you feel about standardizing on 2 * indent as the continuation indent? Considering there's necessarily always a line separator before any continuation indent, that line separator alone substitutes for space as the obligatory whitespace between attributes, even when the indent is 0.

@Ekryd
Copy link
Owner

Ekryd commented Apr 3, 2024

There is a historical reason for the indentation, I couldn't get rid of the + 1 space character from the XML framework that I was using. This was some time ago and I can give it another try 👀

I see that you have a PR already, let me check it out during next weekend

@mches
Copy link
Contributor Author

mches commented Apr 3, 2024

Thanks. I hope you like it. If you determine a new parameter is needed, in order to maintain complete backward compatibility, I'm willing to contribute that.

@mches mches changed the title Unusual choice for schema location continuation indent Revisit schema location indent choice Apr 9, 2024
@Ekryd Ekryd reopened this Apr 19, 2024
@Ekryd
Copy link
Owner

Ekryd commented Apr 19, 2024

Wonderful! I agree that the 2 * indent makes so much more sense. I'll merge this directly.
This might be a breaking change though (for those that use the parameter). All their pom-files will be resorted all of a sudden. Do you think that this should be a new major version of SortPom?

@mches
Copy link
Contributor Author

mches commented Apr 19, 2024

This might be a breaking change though (for those that use the parameter). All their pom-files will be resorted all of a sudden. Do you think that this should be a new major version of SortPom?

In a strict semantic versioning sense, I would say yes. As-is there's not even a config to keep the extra space, except say with the Spotless plugin. For what it's worth, I did notice between 3.3.x and 3.4.0 there was a binary-incompatible change in the sorter due to a changing method signature, which prevented upgrading for me until the next version of Spotless is released. But that signature change wasn't incompatible for the Sortpom plugin itself. All in all, without any backward compatibility mitigations, I think there's a good argument for incrementing major version if you abide by semantic versioning with users' expectations in mind.

@Ekryd
Copy link
Owner

Ekryd commented Apr 20, 2024

Sounds reasonable. Btw what do you use the sorter for? I have some future plans to merge the three SortPom maven modules into one, but if developers are using just the sorter, that might be a bad idea.

@mches
Copy link
Contributor Author

mches commented Apr 21, 2024

Sounds reasonable.

On the other hand, I've used formatters like google-java-format and prettier that don't increment the major version, but rather the minor version, when the only the output changes. Notably both of these formatters are more opinionated and less configurable than SortPom. For these two formatters, the major version changes tend to be reserved for changing the minimum Java version, removing deprecations, removing formatters from core, etc. I'm just one user, but I plan to upgrade to the latest version whether it's a major or minor version upgrade.

Btw what do you use the sorter for?

I'm using it indirectly by way of Spotless. Spotless' Gradle and Maven plugins support formatting Maven POMs. Actually, I added the Gradle support which has not been released yet. SortPomFormatterFunc has a direct dependency on your sorter.

@Ekryd
Copy link
Owner

Ekryd commented May 19, 2024

Released new version

@Ekryd Ekryd closed this as completed May 19, 2024
@mches
Copy link
Contributor Author

mches commented May 19, 2024

Thanks and congrats! I already submitted a PR to Spotless to update to 4.0.0.

@Ekryd
Copy link
Owner

Ekryd commented May 20, 2024

I really appreciate your contributions. I’ll check out the PR

@mches
Copy link
Contributor Author

mches commented Jun 5, 2024

@Ekryd
Copy link
Owner

Ekryd commented Jun 14, 2024

That is great to hear. Please enjoy!!

@Ekryd Ekryd added enhancement released This is now part of the plugin labels Jul 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement released This is now part of the plugin
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants