-
Notifications
You must be signed in to change notification settings - Fork 65
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
Fixing wrong data in certain crash dump fields #15
Conversation
include metadata in crash dumps.
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.
Looks good.
We should really get some test setup going for this. And I don't think something like cargo test or doctrine helps us here (cc @epage): We need to have tests that set up a new crate, build it, and assert that the panic message contains the right info.
quicli uses a setup we can adopt: It has crates in an examples directory and uses assert_cli to test them. It also uses waltz to turn extract these examples files from markdown code blocks, but that's optional.
src/report.rs
Outdated
@@ -26,7 +26,7 @@ pub struct Report { | |||
|
|||
impl Report { | |||
/// Create a new instance. | |||
pub fn new(method: Method, explanation: String) -> Self { | |||
pub fn new<S: Into<String>>(name: S, version: S, method: Method, explanation: String) -> Self { |
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.
This is… weird. What do you actually want to do? Use Cow, &str, or two type params?
Right now, you define one type parameter that will end up being one concrete type that can be converted to a String, and expect the first two function params to be of that one type.
src/report.rs
Outdated
crate_version: env!("CARGO_PKG_VERSION").to_string(), | ||
name: env!("CARGO_PKG_NAME").to_string(), | ||
crate_version: version.into(), | ||
name: name.into(), |
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.
those fields could actually be Cow<'static, str>
. Just saying.
name: &str, | ||
version: &str, | ||
method: Method, | ||
explanation: String, |
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 String here surprises me. It's not wrong, but… I might start a Cow-olution soon!
pub fn new( | ||
name: &str, | ||
version: &str, | ||
method: Method, |
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.
derive Copy for Method pls
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.
To be honest, do we really even need the method anymore?
|
This a 🐛 bug fix.
Currently this is what the crash dump looked for a tool I wanted to use
human-panic
in:This change (using the Metadata object we pass around anyways) fixes this problem