-
Notifications
You must be signed in to change notification settings - Fork 260
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
Add an error field to Record #378
Conversation
@@ -816,6 +818,13 @@ impl<'a> Record<'a> { | |||
self.line | |||
} | |||
|
|||
/// The error associated with the message. |
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 should mention it requires the std feature.
@@ -979,6 +992,14 @@ impl<'a> RecordBuilder<'a> { | |||
self | |||
} | |||
|
|||
/// Set [`error`](struct.Record.html#method.error) |
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.
Same here
Seems reasonable to me to not have a Send bound - even if the fmt::Arguments field wasn't also there you can't really ship a Record around since the error is just borrowed. I do think we should figure out what the macro change looks like before cutting a release with this, though. |
Sure 👍 I might leave this PR sitting open then and switch over to #357 where we can work out how we want the macro to look. I don't think it'll be as risky to change the macros following the same scheme as |
109899e
to
d72abe2
Compare
This has fallen pretty out of date now so I'll close it and revisit in the future. |
For #357
Adds a
dyn Error + 'static
field toRecord
so that concrete errors can be associated with a log event. I'd thought about adding aSend
requirement, but decided against it becauseRecord
already isn'tSend
, there are error types out there folks might want to log that aren'tSend
, and since you can downcast errors if your app needs to send log records to other threads you could use that to downcast and build your own sendable record type.r? @sfackler
Future extension
We could extend the macro to allow something like this in caller code:
Macro support could be added in a fairly naive way to the log macros, or we could take another stab at a tt muncher to support arbitrary special fields before the arguments.