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

Compare _snapshotInfo in Version #1050

Closed
artoonie opened this issue Jun 14, 2023 · 8 comments · Fixed by #1053
Closed

Compare _snapshotInfo in Version #1050

artoonie opened this issue Jun 14, 2023 · 8 comments · Fixed by #1053

Comments

@artoonie
Copy link
Contributor

According to semver, 1.0.0-alpha < 1.0.0-beta.

However, Version.compareTo does not account for _snapshotInfo in its comparison: https://github.com/FasterXML/jackson-core/blob/2.16/src/main/java/com/fasterxml/jackson/core/Version.java#L135

Does it make sense to compare _snapshotInfo as well?

@HEdingfield
Copy link

An easy way to get the desired behavior here would probably be to sort the stuff after the hyphen alphabetically, and whichever is earlier in the list would be considered "older".

@cowtowncoder
Copy link
Member

To be honest, comparability is not used by anything in Jackson itself so it's not a big deal in that sense.

But if anyone wants to fix this, feel free to send a PR; ideally because it solves an actual problem (code outside of Jackson relying on ordering of snaphot info).

@artoonie
Copy link
Contributor Author

This would fix a problem for us. I have created two PRs based on whether you prefer this in 2.16 or master. I recommend closing #1053 and merging #1054.

@cowtowncoder cowtowncoder changed the title Compare _snapshotInfo in Version Compare _snapshotInfo in Version Jun 18, 2023
@cowtowncoder
Copy link
Member

cowtowncoder commented Jun 18, 2023

@artoonie One question: is the comparison in case of "no snapshot info" correct? It seems to me that it would "no snapshot" before snapshot -- which is not correct, I think. That is: as things are, this would be considered ordering:

 2.0.0 < 2.0.0-SNAPSHOT

which is not right I think. I'll see if this is the case or not.

EDIT: I thing I was wrong; behavior is correct as-is. Same as in master (3.0.0-SNAPSHOT). No changes necessary.
And I think this is same as what Maven uses, although cannot be 100% certain (logic within there is rather complicated). So I think what we have are might be correct.

But I would definitely appreciate others looking into this.

cowtowncoder added a commit that referenced this issue Jun 18, 2023
@artoonie
Copy link
Contributor Author

According to semver.org, 2.0.0 is newer than 2.0.0-beta, which is the implemented behavior (but seems to be the opposite of your comment?).

@cowtowncoder
Copy link
Member

cowtowncoder commented Jun 19, 2023

@artoonie Ok yes that makes sense. I was confused by implementation (compareTo() semantics is something I always struggle with). So I think we are all good.

@HEdingfield
Copy link

Thank you both! Any ETA on when 2.16 will be released?

@cowtowncoder
Copy link
Member

Not for quite a while; no ETA. Maybe in August/September?

dmikurube added a commit to embulk/embulk-util-config that referenced this issue Apr 3, 2024
The test has started to fail since Jackson 2.16.0 because of the following changes:

- FasterXML/jackson-core#1053
- FasterXML/jackson-core#1142

These were "fixing" the following issues:

- FasterXML/jackson-core#1050
- FasterXML/jackson-core#1141
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 a pull request may close this issue.

3 participants