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
Including original-sin (panic) in report #43
Conversation
CHANGELOG.md
Outdated
|
||
* 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have a changelog
installed? How is this tool invoked?
src/lib.rs
Outdated
@@ -9,7 +10,8 @@ | |||
//! pretty great at safety, it's not unheard of to access the wrong index in a | |||
//! vector or have an assert fail somewhere. | |||
//! | |||
//! 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The doc comment formatting here seems to have hit a snag.
src/lib.rs
Outdated
@@ -73,7 +76,8 @@ pub struct Metadata { | |||
|
|||
/// `human-panic` initialisation macro | |||
/// | |||
/// 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The doc comment formatting here seems to have hit a snag.
src/lib.rs
Outdated
.expect("human-panic: printing error message to console failed"); | ||
})); | ||
}; | ||
} | ||
|
||
/// 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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? |
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 :)
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-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
featureand 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 🤷