JoshOrndorff / recipes

A Hands-On Cookbook for Aspiring Blockchain Chefs
GNU General Public License v3.0
376 stars 186 forks source link

- replaced uwrap with expect #371

Closed herou closed 3 years ago

herou commented 3 years ago

The title itself is very descriptive. This PR comes from #369 issue.

herou commented 3 years ago

@herou You've replaced the methods, which is good. But the real value of using expect comes from the string that you pass in explaining why the unwrapping couldn't possibly panic. The purpose of this issue is to provide those strings.

For eample:

# Replace this
let a = Some(4);
let b = a.unwrap();

# With this
let a = Some(4);
let b = a.expect("a cannot be a None because I explicitly set it on the previously line.");

Here is an example of where it is already done correctly in the recipes

https://github.com/substrate-developer-hub/recipes/blob/322b2f98f0774638bff1b541ab792fd031b3213c/nodes/kitchen-node/src/service.rs#L51

Got your idea, I just set a default text for now to the expect method in order to pass the tests

herou commented 3 years ago

@wheresaddie, @wheresaddie Since I do not know very well the project I need a bit of help from you guys. The expect("msg to print") method should print meaningful msg. Would be nice if you can provide the list of the msg that you want to print and then I can replace them. Note: The state of this PR should be open!

danforbes commented 3 years ago

Please use GitHub's "Convert to draft" feature for PRs that are not yet ready to merge...it's kind of easy to miss image

JoshOrndorff commented 3 years ago

There's also a label for that. It's useful when you want to CI to run but don't want the PR accidentally merged.

wheresaddie commented 3 years ago

thanks @JoshOrndorff