-
Notifications
You must be signed in to change notification settings - Fork 1k
Check int96 min/max instead of panicking #8603
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
Conversation
Thanks for the submission @rambleraptor. If it's not too much to ask, could you also open an issue for this? The test you added actually errors correctly when your change is reverted. I believe the passed-in min/max needs to be > 12 to trigger the panic. Values with a length less than 12 are correctly caught by I'm not sure why only I'd probably opt for simply getting rid of the asserts, but turning the asserts into tests as you have done is probably the safer way to go. I would, however, revert the change to |
@etseidl I reverted the I completely agree on checking on our side about how the data is created. |
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.
Thanks @rambleraptor. This looks good to me. I notice that there does not appear to be any testing for not enough data, but that impacts all data types and can be a separate PR.
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.
LGTM, pending the recommended test fixes
Made the test changes recommended by @etseidl. I'll leave the all data types test changes to a separate PR. |
Thanks for the improvement @rambleraptor! |
Rationale for this change
Fixes #8614
Part of #7806
We currently panic if the size of an int96 isn't 12. Instead, we should return an error message to avoid a panic.
What changes are included in this PR?
Error message instead of panic.
Are these changes tested?
We typically require tests for all PRs in order to:
If tests are not included in your PR, please explain why (for example, are they covered by existing tests)?
Test included, existing tests should still pass.
Are there any user-facing changes?
None.