Skip to content
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 ensure #469

Merged
merged 4 commits into from
Oct 5, 2021
Merged

Add ensure #469

merged 4 commits into from
Oct 5, 2021

Conversation

ethanfrey
Copy link
Member

Closes #468

ueco-jb
ueco-jb previously approved these changes Oct 4, 2021
Copy link
Contributor

@ueco-jb ueco-jb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personally I'm not a fan, for me this syntax is a bit unintuitive.
It should "sound" like: ensure that {condition}, otherwise {error}, but because there's no "separation" between condition and returned error I need to spend this extra half a second to reverse condition and basically decode macro in head. :)
Long story short, looking at original syntax if {condition} then {error} I read code faster then with proposed macro.

But leaving approve anyway. Code is clean, in some places shorter.

@@ -0,0 +1,34 @@
/// Quick check for a guard. If the condition (first argument) is false,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this a good place for this macro? If we'd commit to using it, basically every contract could use it so some packages/utils seems more appropriate imo.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it belongs in cosmwasm_std. But once it is there it is hard to change.

Putting it here temporarily so we can experiment with it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That said, renaming cw0 to utils makes sense to me

maurolacy
maurolacy previously approved these changes Oct 5, 2021
Copy link
Contributor

@maurolacy maurolacy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like it. Maybe better called ensure_or, but no need to change it.

@ueco-jb
Copy link
Contributor

ueco-jb commented Oct 5, 2021

I actually like Mauro's idea very much, it basically solves my issue and IMO sounds more Rusty.

@ethanfrey
Copy link
Member Author

We could make ensure_or and fail_if which do the opposite actions.

I took ensure! naming from the failure crate.

@ethanfrey ethanfrey dismissed stale reviews from maurolacy and ueco-jb via 98954ab October 5, 2021 09:32
@webmaster128
Copy link
Member

Maybe better called ensure_or, but no need to change it.

Why? ensure!(condition, error) is pretty much how every assertion in every language works. I don't see how it would help.

macro_rules! ensure_eq {
($a:expr, $b:expr, $e:expr) => {
if $a != $b {
return Err($e);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now what if we add

Suggested change
return Err($e);
return Err($e.into());

Wouldn't that open up a huge space of use cases in a very natural way?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought this... but then it requires you to explicitly define the output. Like in the closures in the tests.

Maybe not being able to infer the type is worth it for the flexibility. 🤷

Can you make a PR on top. I am especially interested in the ergonomics of the change in real contract usages, not just my silly test code. It may be able to infer all types in the contracts, so my hesitation is unwarranted.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do. This would make this macro behave like try! and ? when it comes to conversion where you have the same restriction on inference. See

#[macro_export]
#[stable(feature = "rust1", since = "1.0.0")]
#[rustc_deprecated(since = "1.39.0", reason = "use the `?` operator instead")]
#[doc(alias = "?")]
macro_rules! r#try {
    ($expr:expr $(,)?) => {
        match $expr {
            $crate::result::Result::Ok(val) => val,
            $crate::result::Result::Err(err) => {
                return $crate::result::Result::Err($crate::convert::From::from(err));
            }
        }
    };
}

@maurolacy
Copy link
Contributor

Maybe better called ensure_or, but no need to change it.

Why? ensure!(condition, error) is pretty much how every assertion in every language works. I don't see how it would help.

I agree. In fact I mentioned the same in planning today.

@ethanfrey
Copy link
Member Author

Just rebased on main.

Happy to either merge this with future PRs on top to improve, or others to PR on this to improve it.
I do see room for improvement, but I do not want to own the future work.

My main goal was to get something "good enough" into cw-plus utils, that we could iterate on. Either before 0.10.0 or after.

Copy link
Member

@webmaster128 webmaster128 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't get blocked by my suggestion. We can have a second look when migrating it to cosmwasm_std

@ethanfrey ethanfrey merged commit 3739811 into main Oct 5, 2021
@ethanfrey ethanfrey deleted the add-ensure branch October 5, 2021 14:24
@ethanfrey
Copy link
Member Author

I think the suggestion is good.
Happy to merge now.

If someone wants to try this in a new PR, go for it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add ensure! macro
4 participants