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

Relax blanket implementation of Diagnostic #897

Merged
merged 1 commit into from
Jun 25, 2024
Merged

Conversation

calavera
Copy link
Contributor

Issue #, if available:

Fixes #880

Description of changes:

Instead of implementing Diagnostic for everything that implements Display, implement the trait only for a few well known types. This gives people more flexibility to implement Diagnostic.

By submitting this pull request

  • I confirm that my contribution is made under the terms of the Apache 2.0 license.
  • I confirm that I've made a best effort attempt to update all relevant documentation.

Comment on lines 8 to 9
/// `Diagnostic` is automatically derived for types that implement
/// [`Display`][std::fmt::Display]; e.g., [`Error`][std::error::Error].
Copy link

@tannerntannern tannerntannern Jun 23, 2024

Choose a reason for hiding this comment

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

Nit: with these changes I think it's too broad to say that Diagnostic is derived for types that implement Display. Perhaps instead "...for some common types such as std::error::Error"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good call. I've updated the doc.

@calavera calavera force-pushed the into_diagnostic branch 2 times, most recently from 150b3d4 to cdca2ae Compare June 23, 2024 16:02
Instead of implementing Diagnostic for everything that implements Display, implement the trait only for a few well known types. This gives people more flexibility to implement Diagnostic.

Signed-off-by: David Calavera <david.calavera@gmail.com>
@bnusunny
Copy link
Contributor

@calavera is this ready for review?

@calavera
Copy link
Contributor Author

@bnusunny yes. I've tested it manually with a bunch of the examples in the repo and it work fine. I've also tested it with some apps without any issue.

@calavera calavera merged commit cc239ea into main Jun 25, 2024
13 checks passed
@calavera calavera deleted the into_diagnostic branch June 25, 2024 02:57
@TethysSvensson
Copy link
Contributor

This is quite an intrusive change, as it means you can no longer use e.g. anyhow or eyre in your handlers, at least without doing additional boxing.

Would you be open to doing From<...> impls for those crates specifically, as I don't think they would be interesting in adding a dependency for lambda_runtime themselves.

@TethysSvensson
Copy link
Contributor

Also, the requirement for<'b> Into<Diagnostic<'b>> is effectively the same as the requirement Into<Diagnostic<'static>> but less ergonomic.

For instance you will not be able to specify Diagnostic<'static>. This type does implement Into<Diagnostic<'static>>, but does not fulfill the requirement for<'b> Into<Diagnostic<'b>>.

@TethysSvensson
Copy link
Contributor

For anyone else having a hard time upgrading to v0.12 because of this PR, here is my work-around:

struct DiagnosticWrapper(Diagnostic<'static>);

impl<'a> From<DiagnosticWrapper> for Diagnostic<'a> {
    fn from(value: DiagnosticWrapper) -> Self {
        value.0
    }
}

impl std::fmt::Debug for DiagnosticWrapper {
    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
        self.0.fmt(f)
    }
}

impl DiagnosticWrapper {
    fn new<E>(err: E) -> Self
    where
        E: Display,
    {
        DiagnosticWrapper(Diagnostic {
            error_type: Cow::Borrowed(std::any::type_name::<E>()),
            error_message: Cow::Owned(err.to_string()),
        })
    }
}

I then use handler_future.map_err(DiagnosticWrapper::new) inside my service function.

@calavera
Copy link
Contributor Author

I don't think adding dependencies ourselves for those crates makes sense either. I don't know what they return, but I'm guessing the implement the standard Error trait. If that's the case, it should not be necessary to do any conversion. Feel free to test it and send improvements.

@TethysSvensson
Copy link
Contributor

@calavera I've created #901 to continue this discussion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Custom impl Into<Diagnostic> for types that already implement Display
4 participants