-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Improve versioning support documentation and validate version #2214
Improve versioning support documentation and validate version #2214
Conversation
21c2c82
to
ea859e9
Compare
public GsonBuilder setVersion(double ignoreVersionsAfter) { | ||
excluder = excluder.withVersion(ignoreVersionsAfter); | ||
public GsonBuilder setVersion(double version) { | ||
if (Double.isNaN(version) || version < 0.0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have added this < 0.0
check because negative version numbers are probably rather uncommon and this could cause collisions with the undocumented value -1.0
representing no version:
private static final double IGNORE_VERSIONS = -1.0d; |
if (annotationVersion > version) { | ||
return false; | ||
} | ||
return version >= annotationVersion; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should behave equivalently (unless someone uses NaN
as version ...), but is hopefully a bit easier to understand.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, the old code was surprising.
if (annotationVersion <= version) { | ||
return false; | ||
} | ||
return version < annotationVersion; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should behave equivalently (unless someone uses NaN
as version ...), but is hopefully a bit easier to understand.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems reasonable. It's technically an incompatible change, since we never said version numbers could not be negative. But I think it would be pretty surprising if someone were using this with negative version numbers. (For what it's worth, there appears to be exactly one user of GsonBuilder.setVersion
in Google's source code, outside Gson's own tests.)
Resolves #1007
Resolves that issue by considering it a documentation issue, not an implementation issue. When
@Until
was added by 6f59bc3, a corresponding test was also added which checks that the@Until
version is exclusive. From the exclusion standpoint this behavior makes sense since you specify that a field exists until versionX
in which it is removed. This also allows combinations of@Until(X)
marking the old fields and@Since(X)
marking the new fields in a class without overlap. If the value was inclusive it would create situations where for versionX
a field is included, but for versionX.00001
it is not included anymore.Though on the other hand maybe there are situations where it is desired that the value is inclusive, see StackOverflow question on which #1007 is based.