Including original-sin (panic) in report #43
Conversation
spacekookie
changed the title
|
|
||
| * Carrying over metadata from first macro call to properly | ||
| include metadata in crash dumps. | ||
| * Making Method derive Copy (Katharina) |
There was a problem hiding this comment.
Oops oops, it looks like this file was written using an old version of changelog. There was a bug that created these ^M instances which has since been fixed. Could you perhaps try updating and running this again?
There was a problem hiding this comment.
I don't have a changelog installed? How is this tool invoked?
src/lib.rs
Outdated
| //! | ||
| //! When an error eventually occurs, you probably will want to know about it. So | ||
| //! When an error eventually occurs, you proba | ||
| //! #[cfg(feature = "nightly")]bly will want to know about it. So |
There was a problem hiding this comment.
The doc comment formatting here seems to have hit a snag.
src/lib.rs
Outdated
| /// | ||
| /// You can either call this macro with no arguments `setup_panic!()` or | ||
| /// You can either call this macro with no arguments `setup_pan | ||
| /// #[cfg(feature = "nightly")]ic!()` or |
There was a problem hiding this comment.
The doc comment formatting here seems to have hit a snag.
src/lib.rs
Outdated
|
|
||
| /// Utility function that prints a message to our human users | ||
|
|
||
| #[cfg(feature = "nightly")]/// Utility function that prints a message to our human users |
There was a problem hiding this comment.
The doc comment formatting here seems to have hit a snag.
src/lib.rs
Outdated
| }; | ||
|
|
||
| #[cfg(not(feature = "nightly"))] | ||
| let cause = String::from("Not supported by application"); |
There was a problem hiding this comment.
Perhaps we should add slightly more context here? Seeing this message makes me ask the question of: "what isn't supported by the application?"
How about one of:
Error cause could not be determined by the application.Failure cause could not be determined by the application.
yoshuawuyts
left a comment
yoshuawuyts
left a comment
There was a problem hiding this comment.
Added some comments!
|
@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 |
|
Sounds good! Thanks for the review! The doc comment things is really weird, I'll have a look at what might have happened there... |
|
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
changed the title
|
I think I'll just rebase this against |
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 :)
spacekookie
force-pushed
the
original-panic
branch
from
4a5c131 to
fb9ae17
Compare
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-panicon 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
nightlyfeatureand just ommit a static "not supported" or something when running
on stable.
The tests pass (provided you pass
--features nightlyfor now).Wanted to open this PR to discuss how to proceed :)
This fixes #40
This should be a minor bump I believe?
The
Reportstruct isn't exported so adding a field there isn't gonna make a difference 🤷