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

feat!(blanket_impls): impl Diagnostic for &T and Box<T> #367

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

TheLostLambda
Copy link
Contributor

Partially addresses #366

Brings things in line with best practices as described in "Rust for Rustaceans" when it comes to "Ergonomic Trait Implementations" in Chapter 3. Practically means that people can pass more types to any functions taking either dyn Diagnostic or impl Diagnostic.

BREAKING CHANGE: Added blanket impls may overlap with manual user impls. If these impls are just hacks to achieve the same as the blanket ones do, then they can simply be removed. If the impls for T and &T differ semantically, then the user would need to employ the newtype pattern to avoid overlap.

Brings things in line with best practices as described in "Rust for
Rustaceans" when it comes to "Ergonomic Trait Implementations" in Chapter
3. Practically means that people can pass more types to any functions
taking either `dyn Diagnostic` or `impl Diagnostic`.

BREAKING CHANGE: Added blanket impls may overlap with manual user impls.
If these impls are just hacks to achieve the same as the blanket ones do,
then they can simply be removed. If the impls for `T` and `&T` differ
semantically, then the user would need to employ the newtype pattern to
avoid overlap.
@TheLostLambda
Copy link
Contributor Author

TheLostLambda commented Apr 24, 2024

Uh oh, this make have broken more than I thought... Let's hold off for the moment 😅

EDIT: I think it's fine... Turns out typoing unwrap() for unwrap_err() really does make a difference...

@TheLostLambda TheLostLambda marked this pull request as draft April 24, 2024 00:12
@TheLostLambda TheLostLambda marked this pull request as ready for review April 24, 2024 00:17
@TheLostLambda
Copy link
Contributor Author

Confirmed to be working well for me!

This let me go from:

use miette::Diagnostic;
use std::fmt::Debug;

pub trait UnwrapDiagnostic<E> {
    fn unwrap_diagnostic(self) -> E;
}

impl<T: Debug, E: Diagnostic> UnwrapDiagnostic<E> for Result<T, E> {
    fn unwrap_diagnostic(self) -> E {
        self.unwrap_err()
    }
}

impl<T: Debug, E: Diagnostic> UnwrapDiagnostic<E> for Result<T, Box<E>> {
    fn unwrap_diagnostic(self) -> E {
        *self.unwrap_err()
    }
}

macro_rules! assert_miette_snapshot {
    ($diag:expr) => {{
        use insta::{with_settings, assert_snapshot};
        use miette::{GraphicalReportHandler, GraphicalTheme};
        use crate::testing_tools::UnwrapDiagnostic;

        let mut out = String::new();
        GraphicalReportHandler::new_themed(GraphicalTheme::unicode_nocolor())
            .with_width(80)
            .render_report(&mut out, &$diag.unwrap_diagnostic())
            .unwrap();
        with_settings!({
            description => stringify!($diag)
        }, {
            assert_snapshot!(out);
        });
    }};
}

pub(crate) use assert_miette_snapshot;

To just:

macro_rules! assert_miette_snapshot {
    ($diag:expr) => {{
        use insta::{with_settings, assert_snapshot};
        use miette::{GraphicalReportHandler, GraphicalTheme};

        let mut out = String::new();
        GraphicalReportHandler::new_themed(GraphicalTheme::unicode_nocolor())
            .with_width(80)
            .render_report(&mut out, &$diag.unwrap_err())
            .unwrap();
        with_settings!({
            description => stringify!($diag)
        }, {
            assert_snapshot!(out);
        });
    }};
}

pub(crate) use assert_miette_snapshot;

@zkat zkat added the breaking A semver-major breaking change label Apr 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking A semver-major breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants