Skip to content
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 ErrorRef wrapper #327

Merged
merged 1 commit into from
Jan 2, 2024
Merged

Conversation

demurgos
Copy link
Contributor

@demurgos demurgos commented Dec 14, 2023

This commit adds the ErrorRef wrapper type. It enables to log error by reference, without moving ownership. It means that it is the non-owning counterpart of ErrorValue.

Logging error references is an important use case for transparent logging middlewares. With this wrapper type, you can log the error and still pass it up the call stack.

Such a wrapper type could already easily be implemented in user applications, but having it as part of Slog should help ergonomics. This also enables further improvements by integrating it with the Slog macros.

Closes #288

Make sure to:

  • Add an entry to CHANGELOG.md (if necessary)

This commit adds the `ErrorRef` wrapper type. It enables to log error by reference, without moving ownership. It means that it is the non-owning counterpart of `ErrorValue`.

Logging error references is an important use case for transparent logging middlewares. With this wrapper type, you can log the error and still pass it up the call stack.

Such a wrapper type could already easily be implemented in user applications, but having it as part of Slog should help ergonomics. This also enables further improvements by integrating it with the Slog macros.

Closes slog-rs#288
@demurgos
Copy link
Contributor Author

Unfortunately this does not support dyn Error. I'll see if I can find something to help with it.

@Techcable Techcable self-assigned this Jan 2, 2024
@Techcable Techcable added C-enhancement Category: An issue proposing an enhancement or a PR with one. P-medium Priority: Medium labels Jan 2, 2024
@Techcable
Copy link
Member

Thank you! Sorry for the delayed review.

I will either open an issue or add a test for supporting dyn Error.

Copy link
Member

@Techcable Techcable left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is good enough for publication. I'll maybe make one or two changes/comments.

src/tests.rs Show resolved Hide resolved
key: Key,
serializer: &mut dyn Serializer,
) -> Result {
serializer.emit_error(key, self.0)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should add an emit_error_ref method with a default implementation?

It is unfortunate we have the 'static bound, but that's probably a requirement for things like slog_async. An alternative would be using format_args! and emit_arguments, but that would loose information 😦.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good point, may be worth adding it in the future

@Techcable Techcable merged commit a50e813 into slog-rs:master Jan 2, 2024
35 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-enhancement Category: An issue proposing an enhancement or a PR with one. P-medium Priority: Medium
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ErrorValue requires the wrapped error to be 'static
2 participants