search

rust-cli / human-panic

Panic messages for humans.
https://docs.rs/human-panic
Apache License 2.0
1.66k stars 65 forks source link

Including original-sin (panic) in report #43

Closed spacekookie closed 6 years ago

spacekookie commented 6 years ago

Choose one: This a 🐛 bug fix

Including original panic-cause in report

Okay so this PR is a bit of a dilemma. The API we need to get the original panic message isn't stable and requires a feature flag. We can conditionally enable the feature flag for nightly users but that doesn't quite deal with the problem of what to do when someone is using human-panic on stable.

As far as I can see there's no other API to get that information? We could always just gate it all away behind the nightly feature and just ommit a static "not supported" or something when running on stable.

The tests pass (provided you pass --features nightly for now). Wanted to open this PR to discuss how to proceed :)

This fixes #40

This should be a minor bump I believe?

The Report struct isn't exported so adding a field there isn't gonna make a difference 🤷

yoshuawuyts commented 6 years ago

@spacekookie I think being able to report on nightly, rather than not at all seems reasonable. We can always move to upgrade from there once more functionality around this stabilizes.

If we decide to merge this patch we should probably open a new issue to move the same reporting to stable, and link it against any stabilization issues on rust-lang/rust so we can switch over once it stabilizes. :sparkles:

spacekookie commented 6 years ago

Sounds good! Thanks for the review!

The doc comment things is really weird, I'll have a look at what might have happened there...

spacekookie commented 6 years ago

Ping @yoshuawuyts sorry for the delay, I fixed the issues in the code. No idea what caused the issue in the changelog, or how to fix it? What's the tool that is invoked there?

spacekookie commented 6 years ago

I think I'll just rebase this against master once #49 lands :tada: