-
Notifications
You must be signed in to change notification settings - Fork 37
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
Guard functions #1243
Guard functions #1243
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.
Other than the comments I left it looks good.
console.log('nonNullOkValue called'); | ||
// @ts-ignore | ||
return { Ok: 'Something other than null' }; | ||
return 'Upgrades to this canister are disabled'; |
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.
Shouldn't this throw
?
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.
Not needed I'll just delete it in my next 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.
It seems like many of these edge cases aren't valid with the new syntax. However, I wonder if there are some that still are, or if we have even different edge cases. I like that this file gets us thinking about all the ways users might do something wrong. For example, what if they did @query([], bool, { guard: someInvalidIdentifier })
, what error would they get? I just want to make sure that users get direct, informative error messages when they do something wrong, so I would suggest that we modify this file for the scenarios that make sense with our new syntax, or that we create an issue for improving these errors later.
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'm just not sure this makes sense, an invalid identifier would show up as a typescript or javascript error, we don't do many of the things we used to do so I would say it's more likely not our concern.
Generally we want errors to show nicely but it's so different now.
quote! { | ||
#[ic_cdk_macros::query(manual_reply = true, composite = #is_composite)] | ||
#[ic_cdk_macros::query(manual_reply = true, composite = #is_composite #guard_attribute)] |
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.
Is there supposed to be comma between composite and #guard_attribute? I'm sure you did it right but it caught my eye so just double check.
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 correct, the comma is in the guard_attribute
Closes #1209