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 eyre::Ok #91

Merged
merged 1 commit into from
Nov 26, 2023
Merged

Add eyre::Ok #91

merged 1 commit into from
Nov 26, 2023

Conversation

kylewlacy
Copy link
Contributor

This PR is a backport of the anyhow::Ok function (dtolnay/anyhow#192). For motivation, consider the following minimal example:

fn main() -> eyre::Result<()> {
    let f = || eyre::Ok(());

    f()?;

    Ok(())
}

Without this Ok, function, the next best way to write line 2 would be let f = || Result::<_, eyre::Error>::Ok(());, which is quite verbose! anyhow::Ok is probably the biggest pain point I've had in the cases where I've used eyre in place of anyhow.

@thenorili
Copy link
Contributor

I'm struggling to understand the use case of this change, but I understand it's an anyhow feature you rely on and that somehow it works better with type inference when getting an Ok() at least though, and that compatibility with anyhow is worthwhile.

A search returns many callsites and uses of Ok(()) in the code, tests, and documentation that seem to be implicitly Result::Ok().

Can you help me understand why you've specified Result::Ok() in a couple places and not others? I wouldn't think Result::Ok() would actually specify much between core::fmt::Result and eyre::Result.

@ten3roberts ten3roberts added the C-enhancement Category: New feature or request label Nov 7, 2023
@kylewlacy
Copy link
Contributor Author

Can you help me understand why you've specified Result::Ok() in a couple places and not others? I wouldn't think Result::Ok() would actually specify much between core::fmt::Result and eyre::Result.

It's... been a while since I made this PR, so I've lost a lot of the context. If I remember right, my intent was to make sure that code that was using the function std::Result::Ok(...) was still using the same function std::Result::Ok(...), but looking back, it actually doesn't do that (since eyre::Result shadows std::Result). Since this PR has conflicts, I'll take a second stab at it (preferring just to use the bare Ok function where possible).

@thenorili
Copy link
Contributor

Can you help me understand why you've specified Result::Ok() in a couple places and not others? I wouldn't think Result::Ok() would actually specify much between core::fmt::Result and eyre::Result.

It's... been a while since I made this PR, so I've lost a lot of the context. If I remember right, my intent was to make sure that code that was using the function std::Result::Ok(...) was still using the same function std::Result::Ok(...), but looking back, it actually doesn't do that (since eyre::Result shadows std::Result). Since this PR has conflicts, I'll take a second stab at it (preferring just to use the bare Ok function where possible).

I appreciate you giving a second stab at it ^^; eyre has been a little out of water but there are a few more folks around now so we're pumping out the backlog! I think the actual conflict here was nothing, it just needed an auto-resolve and I finally got the workflow to work for that on my side : ) I'd really appreciate you checking that the actual types are correct and and not over or underspecified though.

Good luck with this, if you get stuck you can feel free to ping me and ask for help!

@thenorili
Copy link
Contributor

error[E0308]: mismatched types
   --> eyre/src/lib.rs:712:9
    |
703 |     ) -> core::fmt::Result {
    |          ----------------- expected `std::result::Result<(), std::fmt::Error>` because of return type
...
712 |         Ok(())
    |         ^^^^^^ expected `Result<(), Error>`, found `Result<(), Report>`
    |
    = note: expected enum `std::result::Result<_, std::fmt::Error>`        
               found enum `std::result::Result<_, Report>`

error[E0308]: mismatched types
   --> eyre/src/lib.rs:834:9
    |
790 |     ) -> core::fmt::Result {
    |          ----------------- expected `std::result::Result<(), std::fmt::Error>` because of return type
...
834 |         Ok(())
    |         ^^^^^^ expected `Result<(), Error>`, found `Result<(), Report>`
    |
    = note: expected enum `std::result::Result<_, std::fmt::Error>`        
               found enum `std::result::Result<_, Report>`

This is the error that wanted the Result::Ok(()) specification. I think that it figures out the type this way because Result on the return type implies core::fmt::Result but Ok() implies the function defined in the same top-level scope. Now that I've looked at it it probably is fine as is, I don't think it should break other people's stuff since they aren't importing eyre::Ok() by default like we are. LGTM.

Copy link
Contributor

@thenorili thenorili left a comment

Choose a reason for hiding this comment

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

LGTM, shouldn't break anyone else's stuff unless they're using eyre::*

@thenorili thenorili merged commit da84e8c into eyre-rs:master Nov 26, 2023
29 checks passed
@kylewlacy kylewlacy deleted the add-ok-fn branch November 27, 2023 08:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-enhancement Category: New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants