-
Notifications
You must be signed in to change notification settings - Fork 67
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
Add assert_with_error! macro #749
Conversation
soroban-sdk/src/lib.rs
Outdated
/// | ||
/// See [`contracterror`] for how to define an error type. | ||
#[macro_export] | ||
macro_rules! assert_error { |
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 chose the name assert_error
because it parallels panic_error
(which parallels panic_any
). I don't love the name assert_error
, because it reads more like we're asserting that an error should occur, rather than creating an assertion that will cause an error if false. I've gone with consistency, but open to suggestions.
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 find it a little misleading. You are not asserting that there is an error, rather the opposite. For near's sdk, they went with require
, which I would advocate for.
Also I can't wait until macro's can be bound to types! env.require!(...)
reads better to me.
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.
Also I can't wait until macro's can be bound to types!
env.require!(...)
reads better to me.
Is this in development? Do you have a link to the tracking PR / RFC? I can't find it.
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.
It's called postfix macros polyfill here.
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.
@graydon suggested assert_with_error
or assert_or_error
.
On it's own I think I prefer assert_or_error
over require
because it retains the assert vocab. i.e.:
assert_or_error
panic_error
If we wanted panic_error to also follow, then the _with_error
variant would probably fit better, i.e.:
assert_with_error
panic_with_error
.
Thoughts? cc @willemneal @graydon @paulbellamy
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.
*_with_error
makes sense to me.
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.
Follow up: #752
### What Rename `panic_error!` to `panic_with_error!`. ### Why For consistency with `assert_with_error!` introduced in #749.
What
Add
assert_with_error!
macro that works likeassert!
but usespanic_error!
.Why
I've noticed that reviewing examples written by the folks experimenting with Soroban that folks like to use
assert!
. We provide apanic_error!
function for folks who like to panic on error, so that they can panic with an error code instead of a string. Given the patterns I'm seeing withassert!
I think it's worth also providing same for error codes, and seeing if folks find error codes preferrable with the same assertion pattern.