-
Notifications
You must be signed in to change notification settings - Fork 450
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 example usage comments to EnvAccess
methods
#797
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.
I like this PR very much and would love to have examples for the important self.env()
API. However, some examples are currently flawed from what I can see and shouldn't/won't compile. Especially the #[ink(message)]
examples are super useful but they need a lot of hidden context around them in order to make them compile properly. Non-compiling doc tests should be avoided at all cost since they are a nightmare for maintainability.
crates/lang/src/env_access.rs
Outdated
/// let message = ink_prelude::format!("contract's account id is {:?}", account_id); | ||
/// ink_env::debug_println(&message); |
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.
We should use the upcoming debug println macros here as soon as they are available imo.
I updated the PR, the code in the doc comment examples is compiled now. So please give it another round of review. I briefly investigated using a proc. macro to reduce the code duplication, but quickly ran into issues with generating only part of a doc comment. |
This reverts commit f731912.
Yep, this is exactly what I expected unfortunately. I think for now we cannot do much about this. |
I updated the doc tests with some more vivid examples. Also I had to fix #801 as part of the recent commits. I used your suggestion from that issue:
|
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.
Seeing code like
ink_env::account_id::<T>().expect("couldn't decode contract account ID")
all over the place I come to believe we need a follow-up PR that replaces those expect
calls with unwrap_or_else(|error| panic!("couldn't decode contract account ID: {}", error))
that provides more information about the underlying error.
Co-authored-by: Robin Freyler <[email protected]>
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
Reminder: We should create an issue for the follow-up PR for #797 (review).
Closes #644.