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

Improve expect messages for decoding failure #840

Closed
cmichi opened this issue Jul 3, 2021 · 9 comments
Closed

Improve expect messages for decoding failure #840

cmichi opened this issue Jul 3, 2021 · 9 comments
Assignees
Labels
A-examples [examples] Work item good first issue Good for newcomers

Comments

@cmichi
Copy link
Collaborator

cmichi commented Jul 3, 2021

Follow-up for #797.

Copy/Pasted from @Robbepop's comment there:

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.

@cmichi cmichi added good first issue Good for newcomers A-examples [examples] Work item labels Jul 3, 2021
@gangov
Copy link
Contributor

gangov commented Sep 30, 2022

is this still available?

@cmichi
Copy link
Collaborator Author

cmichi commented Oct 1, 2022

@gangov Yes, feel free to assign it to yourself 👍.

@gangov
Copy link
Contributor

gangov commented Oct 3, 2022

thanks, a couple of things to clear if that's okay:

  1. I cannot assign the issue to myself
  2. There are a dozen places in the code where we cannot use .unwrap_or_else because we're dealing with Option. The error we have is the same all-over-the-place
    error[E0593]: closure is expected to take 0 arguments, but it takes 1 argument
    What would be the best approach here?
  3. Should we include the files in the examples directory and the ones for the tests?

@cmichi
Copy link
Collaborator Author

cmichi commented Oct 27, 2022

Sorry @gangov, the notification for your comment fell under the table.

  1. Are you still interested in it? I'd assign it to you then.
  2. You can ignore Option.
  3. Tests no, examples directory yes.

@gangov
Copy link
Contributor

gangov commented Oct 27, 2022

hey @cmichi, no worries about the notification. Yes, I'd like to give it a try, please assign it to me. I'll ignore Option and tests, but then follow up on the rest as per the initial comment

@gangov
Copy link
Contributor

gangov commented Oct 29, 2022

hey, @cmichi I created the PR a couple of minutes ago, but I did it from a forked repo. Please let me know if that troubles you in some way to review, so I can adjust

Edit: I've deleted the old PR, making a new

@gangov
Copy link
Contributor

gangov commented Oct 30, 2022

hey again @cmichi this is the PR #1456

There were some cases in which we had tests in the examples, so I didn't change those, please let me know if that needs an update, too

@gangov
Copy link
Contributor

gangov commented Nov 4, 2022

hey @cmichi the above mentioned PR has been merged, I guess we can close this one

@HCastano
Copy link
Contributor

Closed by #1456

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-examples [examples] Work item good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

3 participants