-
Notifications
You must be signed in to change notification settings - Fork 107
GH-125: Allow null timestamp holder sans timezone #941
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
base: main
Are you sure you want to change the base?
Conversation
This comment has been minimized.
This comment has been minimized.
|
Please set labels, I do not have permission |
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.
I think it'd be more natural to move the else block below as the second conditional check, so we aren't repeating boolean clauses.
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.
I agree, it looks more "logical".
| "holder.timezone: %s not equal to vector timezone: %s", | ||
| holder.timezone, this.timeZone)); | ||
| } else if (holder.isSet > 0) { | ||
| if (!this.timeZone.equals(holder.timezone)) { |
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 looks better to me.
The build fails with this issue, I do not see how it is related to my changes. Do I need to add anything else to get this PR merged? |
Description
Fixes an
IllegalArgumentExceptioninTimeStamp*TZVector.set/setSafewhen unsetting values using a holder with anulltimezone. The validation logic now correctly ignores the timezone check whenholder.isSet <= 0, allowing default-constructed holders to be used for unsetting values as expected.Comprehensive tests added for all timestamp precisions (Micro, Milli, Nano, Sec) to verify the fix and ensure the existing workaround (setting explicit timezone) remains supported.
Closes #125 .