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

feat: Print unexpected panics to standard error #601

Merged
merged 1 commit into from
Mar 29, 2019

Conversation

drager
Copy link
Member

@drager drager commented Mar 23, 2019

This will print to standard error for unexpected panics and then let human_panic handle panics, just like before.

This will fix #562.

This will print to standard error for unexpected panics
and then let `human_panic` handle panics, just like before.
Copy link
Member

@ashleygwilliams ashleygwilliams left a comment

Choose a reason for hiding this comment

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

thanks so much for this @drager! is there any way to test this? (i'm not sure..) could you show an example of what this looks like when a panic happens?

// Then call human_panic.
let file_path = human_panic::handle_dump(&meta, info);
human_panic::print_msg(file_path, &meta)
.expect("human-panic: printing error message to console failed");
Copy link
Member

Choose a reason for hiding this comment

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

if you'd like to use ? here instead of expect we can add returning a Result from main to this PR in another commit! if not, that's OK :)

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'm not sure we can do that since set_hook wont accept a Result as a return type for the hook. It only accepts ().

@drager
Copy link
Member Author

drager commented Mar 23, 2019

@ashleygwilliams Yeah, you could do this by just panicing somewhere in the wasm-pack codebase. Here's what it looks like:

image

@ashleygwilliams ashleygwilliams changed the title feat: Print unexpected panics to standard error [WIP] feat: Print unexpected panics to standard error Mar 23, 2019
@ashleygwilliams
Copy link
Member

per convo in discord, @drager is gonna do the refactor of main to return a result by monday so we can land this in the 0.8.0 release i intend to do on that day. if not, we can land this as is for the release and refactor later :)

@drager
Copy link
Member Author

drager commented Mar 27, 2019

@ashleygwilliams I'm not sure we can refactor that expect do that since set_hook wont accept a Result as a return type for the hook. It only accepts ().

@ashleygwilliams
Copy link
Member

@drager let's just land this as is then!

@ashleygwilliams ashleygwilliams changed the title [WIP] feat: Print unexpected panics to standard error feat: Print unexpected panics to standard error Mar 29, 2019
@ashleygwilliams ashleygwilliams removed the work in progress do not merge! label Mar 29, 2019
@ashleygwilliams ashleygwilliams merged commit 67089e0 into rustwasm:master Mar 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unexpected panics should still print the full panic message
2 participants