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

FELIX-6736 Emit properties in alphabetical order #345

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

kwin
Copy link
Member

@kwin kwin commented Nov 15, 2024

No description provided.

@kwin
Copy link
Member Author

kwin commented Nov 15, 2024

@cziegeler Please have a look, this test fails without sorting the keys first.

@kwin kwin force-pushed the feature/FELIX-6736-sort-properties-alphabetically-in-ConfigurationWriter branch from bf51840 to ab01dae Compare November 15, 2024 14:37
@@ -16,7 +16,7 @@
* specific language governing permissions and limitations
* under the License.
*/
@Version("1.0.0")
@Version("1.1.0")
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like to increase the minor version here in order to ease depending on this fixed version.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As you only changed internal behavior, package versions are not for "easing" anything but to allow semantic versioning so increasing the minor is wrong here.

Suggested change
@Version("1.1.0")
@Version("1.0.1")

Copy link
Member Author

@kwin kwin Nov 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a potentially backward incompatible semantical change (affecting consumers). Just a change which is not detected by baselining because the method signature did not change. Therefore adjusting the minor version is IMHO the right thing to do!

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it affects consumers you need a major version bump.

@kwin kwin force-pushed the feature/FELIX-6736-sort-properties-alphabetically-in-ConfigurationWriter branch from ab01dae to fdaf9c6 Compare November 15, 2024 14:39
@cziegeler cziegeler requested review from cziegeler and removed request for cziegeler November 15, 2024 14:45
@cziegeler
Copy link
Contributor

It is expected that the tests fail, the configuration object preserves insertion order - if you read and write the same feature without changes, it should be identical (maybe minus whitespaces, indention).

@kwin
Copy link
Member Author

kwin commented Nov 15, 2024

I just aligned with the logic from

for ( Enumeration ce = orderedKeys(properties); ce.hasMoreElements(); )
. But I am also fine with making this an opt-in behaviour.

@cziegeler
Copy link
Contributor

ConfigurationHandler is not really a public API. The configuration writer is used for the various feature model implementations and it would be pretty annoying if it would destroy out of the sudden the order of things. So yes, opt-in makes sense to me.
A different option is to have a utility method that has a Dictionary as an input and outputs a sorted Dictionary. This could then be used everywhere, not just when writing out configurations.

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

Successfully merging this pull request may close these issues.

3 participants