-
Notifications
You must be signed in to change notification settings - Fork 1.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
Replace usages of internal_err with exec_err where appropriate #9241
Conversation
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 @Omega359 for taking this, it looks good for me, the only tiny improvement is to use not_impl_err!
for Not Supported Errors
…e/internal_err_9164
…nsupported type errors." This reverts commit fe0517a.
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.
Thank you @Omega359 -- this looks great and I think we should get this asap as it will conflict with many of the function extraction PRs.
I took the liberty of merging up from main and resolving one such conflict and plan to merge this when the CI checks have passed
@@ -58,7 +58,7 @@ impl NullIfFunc { | |||
Self { | |||
signature: | |||
Signature::uniform(2, SUPPORTED_NULLIF_TYPES.to_vec(), | |||
Volatility::Immutable, | |||
Volatility::Immutable, |
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 is a strange change -- I wonder if cargo fmt doesn't see this file for some reason 🤔
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.
Which issue does this PR close?
Closes #9164
Rationale for this change
Code cleanup.
What changes are included in this PR?
Only internal_err! for incorrect arg count or data type was migrated to exec_err, all other cases were left alone.
Are these changes tested?
Yes,
Are there any user-facing changes?
No.