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

Make ContextCompat consistent and non-ambiguous with WrapErr #150

Merged
merged 8 commits into from
Jun 28, 2024
33 changes: 13 additions & 20 deletions eyre/src/context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -61,42 +61,34 @@ where
Err(e) => Err(e.ext_report(msg())),
}
}
}

#[cfg(feature = "anyhow")]
fn context<D>(self, msg: D) -> Result<T, Report>
#[cfg(feature = "anyhow")]
impl<T, E> crate::ContextCompat<T> for Result<T, E>
where
Self: WrapErr<T, E>,
{
#[track_caller]
fn context<D>(self, msg: D) -> crate::Result<T, Report>
where
D: Display + Send + Sync + 'static,
{
self.wrap_err(msg)
}

#[cfg(feature = "anyhow")]
fn with_context<D, F>(self, msg: F) -> Result<T, Report>
#[track_caller]
fn with_context<D, F>(self, f: F) -> crate::Result<T, Report>
where
D: Display + Send + Sync + 'static,
F: FnOnce() -> D,
{
self.wrap_err_with(msg)
self.wrap_err_with(f)
}
}

#[cfg(feature = "anyhow")]
impl<T> crate::ContextCompat<T> for Option<T> {
fn wrap_err<D>(self, msg: D) -> Result<T, Report>
where
D: Display + Send + Sync + 'static,
{
self.context(msg)
}

fn wrap_err_with<D, F>(self, msg: F) -> Result<T, Report>
where
D: Display + Send + Sync + 'static,
F: FnOnce() -> D,
{
self.with_context(msg)
}

#[track_caller]
fn context<D>(self, msg: D) -> Result<T, Report>
where
D: Display + Send + Sync + 'static,
Expand All @@ -107,6 +99,7 @@ impl<T> crate::ContextCompat<T> for Option<T> {
}
}

#[track_caller]
fn with_context<D, F>(self, msg: F) -> Result<T, Report>
where
D: Display + Send + Sync + 'static,
Expand Down
31 changes: 2 additions & 29 deletions eyre/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1125,21 +1125,6 @@ pub trait WrapErr<T, E>: context::private::Sealed {
where
D: Display + Send + Sync + 'static,
F: FnOnce() -> D;

/// Compatibility re-export of wrap_err for interop with `anyhow`
#[cfg(feature = "anyhow")]
#[cfg_attr(track_caller, track_caller)]
fn context<D>(self, msg: D) -> Result<T, Report>
where
D: Display + Send + Sync + 'static;

/// Compatibility re-export of wrap_err_with for interop with `anyhow`
#[cfg(feature = "anyhow")]
#[cfg_attr(track_caller, track_caller)]
fn with_context<D, F>(self, f: F) -> Result<T, Report>
where
D: Display + Send + Sync + 'static,
F: FnOnce() -> D;
}

/// Provides the [`ok_or_eyre`][OptionExt::ok_or_eyre] method for [`Option`].
Expand Down Expand Up @@ -1197,7 +1182,8 @@ pub trait OptionExt<T>: context::private::Sealed {
M: Debug + Display + Send + Sync + 'static;
}

/// Provides the `context` method for `Option` when porting from `anyhow`
/// Provides the `context` and `with_context` methods for `Result` and `Option` to enhance
/// compatibility when porting from anyhow.
///
/// This trait is sealed and cannot be implemented for types outside of
/// `eyre`.
Expand Down Expand Up @@ -1256,19 +1242,6 @@ pub trait ContextCompat<T>: context::private::Sealed {
where
D: Display + Send + Sync + 'static,
F: FnOnce() -> D;

/// Compatibility re-export of `context` for porting from `anyhow` to `eyre`
#[cfg_attr(track_caller, track_caller)]
fn wrap_err<D>(self, msg: D) -> Result<T, Report>
where
D: Display + Send + Sync + 'static;

/// Compatibility re-export of `with_context` for porting from `anyhow` to `eyre`
#[cfg_attr(track_caller, track_caller)]
fn wrap_err_with<D, F>(self, f: F) -> Result<T, Report>
where
D: Display + Send + Sync + 'static,
F: FnOnce() -> D;
}

/// Equivalent to Ok::<_, eyre::Error>(value).
Expand Down
8 changes: 6 additions & 2 deletions eyre/tests/test_location.rs
Original file line number Diff line number Diff line change
Expand Up @@ -100,12 +100,14 @@
#[cfg(feature = "anyhow")]
#[test]
fn test_context() {
use eyre::ContextCompat;
Copy link
Contributor

Choose a reason for hiding this comment

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

For consistency with the other tests, this use could be moved to after the hook setup, just before using ContextCompat::with_context.

Or the existing uses moved to the start of each test fn as well.

Examples of use right before trait usage:


let _ = eyre::set_hook(Box::new(|_e| {
let expected_location = file!();
Box::new(LocationHandler::new(expected_location))
}));

use eyre::WrapErr;

Check failure on line 110 in eyre/tests/test_location.rs

View workflow job for this annotation

GitHub Actions / Clippy (stable)

unused import: `eyre::WrapErr`

Check warning on line 110 in eyre/tests/test_location.rs

View workflow job for this annotation

GitHub Actions / Test Suite (stable)

unused import: `eyre::WrapErr`

Check warning on line 110 in eyre/tests/test_location.rs

View workflow job for this annotation

GitHub Actions / Test Suite (stable, --all-features)

unused import: `eyre::WrapErr`

Check warning on line 110 in eyre/tests/test_location.rs

View workflow job for this annotation

GitHub Actions / Test Suite (stable, --features pyo3)

unused import: `eyre::WrapErr`

Check warning on line 110 in eyre/tests/test_location.rs

View workflow job for this annotation

GitHub Actions / Test Suite (ubuntu-latest)

unused import: `eyre::WrapErr`

Check warning on line 110 in eyre/tests/test_location.rs

View workflow job for this annotation

GitHub Actions / Test Suite (macOS-latest)

unused import: `eyre::WrapErr`

Check warning on line 110 in eyre/tests/test_location.rs

View workflow job for this annotation

GitHub Actions / Test Suite (windows-latest)

unused import: `eyre::WrapErr`
let err = read_path("totally_fake_path")
.context("oopsie")
.unwrap_err();
Expand All @@ -117,12 +119,14 @@
#[cfg(feature = "anyhow")]
#[test]
fn test_with_context() {
use eyre::ContextCompat;
Copy link
Contributor

Choose a reason for hiding this comment

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

As above:

For consistency with the other tests, this use could be moved to after the hook setup, just before using ContextCompat::with_context.

Or the existing uses moved to the start of each test fn as well.

Examples of use right before trait usage:


let _ = eyre::set_hook(Box::new(|_e| {
let expected_location = file!();
Box::new(LocationHandler::new(expected_location))
}));

use eyre::WrapErr;

Check failure on line 129 in eyre/tests/test_location.rs

View workflow job for this annotation

GitHub Actions / Clippy (stable)

unused import: `eyre::WrapErr`

Check warning on line 129 in eyre/tests/test_location.rs

View workflow job for this annotation

GitHub Actions / Test Suite (stable)

unused import: `eyre::WrapErr`

Check warning on line 129 in eyre/tests/test_location.rs

View workflow job for this annotation

GitHub Actions / Test Suite (beta, --all-features)

unused import: `eyre::WrapErr`

Check warning on line 129 in eyre/tests/test_location.rs

View workflow job for this annotation

GitHub Actions / Test Suite (beta)

unused import: `eyre::WrapErr`

Check warning on line 129 in eyre/tests/test_location.rs

View workflow job for this annotation

GitHub Actions / Test Suite (stable, --all-features)

unused import: `eyre::WrapErr`

Check warning on line 129 in eyre/tests/test_location.rs

View workflow job for this annotation

GitHub Actions / Test Suite (stable, --features pyo3)

unused import: `eyre::WrapErr`

Check warning on line 129 in eyre/tests/test_location.rs

View workflow job for this annotation

GitHub Actions / Test Suite (nightly, --all-features)

unused import: `eyre::WrapErr`

Check warning on line 129 in eyre/tests/test_location.rs

View workflow job for this annotation

GitHub Actions / Test Suite (ubuntu-latest)

unused import: `eyre::WrapErr`

Check warning on line 129 in eyre/tests/test_location.rs

View workflow job for this annotation

GitHub Actions / Test Suite (macOS-latest)

unused import: `eyre::WrapErr`

Check warning on line 129 in eyre/tests/test_location.rs

View workflow job for this annotation

GitHub Actions / Test Suite (windows-latest)

unused import: `eyre::WrapErr`
let err = read_path("totally_fake_path")
.with_context(|| "oopsie")
.unwrap_err();
Expand All @@ -140,7 +144,7 @@
}));

use eyre::ContextCompat;
let err = None::<()>.wrap_err("oopsie").unwrap_err();
let err = None::<()>.context("oopsie").unwrap_err();
Copy link
Contributor

Choose a reason for hiding this comment

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

The encosing test test_option_compat_wrap_err might better be renamed to test_option_compat_context?

Copy link
Contributor

@LeoniePhiline LeoniePhiline Jun 28, 2024

Choose a reason for hiding this comment

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

test_option_compat_context and test_option_compat_with_context already exist and are (now) identical to test_option_compat_wrap_err and test_option_compat_wrap_err_with.

The tests test_option_compat_wrap_err and test_option_compat_wrap_err_with are obsolete and should be deleted.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And this is why consistent naming is so good, cause now that yields a duplicate definition error.

Thanks a lot for your suggestions 😊


// should panic if the location isn't in our crate
println!("{:?}", err);
Expand All @@ -155,7 +159,7 @@
}));

use eyre::ContextCompat;
let err = None::<()>.wrap_err_with(|| "oopsie").unwrap_err();
let err = None::<()>.with_context(|| "oopsie").unwrap_err();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This correctly adheres to our model now as you can not "wrap an error" over an option.

You now either use the anyhow compatibility flag, or as the AnyhowCompat suggests the std methods like ok_or_else etc

Copy link
Contributor

Choose a reason for hiding this comment

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

The encosing test test_option_compat_wrap_err_with might better be renamed to test_option_compat_with_context?


// should panic if the location isn't in our crate
println!("{:?}", err);
Expand Down
Loading