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 ErrContext method to Result #51

Closed
wants to merge 1 commit into from
Closed

Conversation

knpwrs
Copy link

@knpwrs knpwrs commented Aug 17, 2024

This feature proposal is inspired by the extremely popular Rust library, anyhow. One of the features of anyhow is that you can attach context to errors to assist in troubleshooting.

For instance, an error like No such file or directory can be hard to debug without more context about what higher level step the application was in the middle of.

The following code with anyhow...

use anyhow::{Context, Result};

fn main() -> Result<()> {
    ...
    it.detach().context("Failed to detach the important thing")?;

    let content = std::fs::read(path)
        .with_context(|| format!("Failed to read instrs from {}", path))?;
    ...
}

Results in the following error:

Error: Failed to read instrs from ./path/to/instrs.json

Caused by:
    No such file or directory (os error 2)

@ccoVeille
Copy link

I wouldn't use context in the func name. Go context is a widely used notion, I wouldn't be happy as a Gopher to find something named context, that is not about Go context

And clearly, I wouldn't use context as a variable name, as it would lead to import shadowing.

And about the feature, I'm unsure how it differs from a fmt.Errorf directly used

@ccoVeille
Copy link

ccoVeille commented Aug 18, 2024

I mean Result is an error

https://pkg.go.dev/github.com/samber/mo#Result.Error

So calling fmt.Errorf("%w whatever", r.Error()) is enough, no?

@knpwrs
Copy link
Author

knpwrs commented Aug 18, 2024

I am rather new to go and those are all excellent points.

So calling fmt.Errorf("%w whatever", r.Error()) is enough, no?

If I am understanding correctly, the difference is that the new error information would bubble up to where the error is actually handled, rather than having to handle the errors deeper down in the call stack. You can handle errors at higher levels in the call stack and still get context as to where the actual error occurred.

@ccoVeille
Copy link

That's the idea behind error wrapping in Go

Please note your code is not far from what I suggested.

So it's more a matter of "do we need such method to do it?" than "is it something useful?" It's definitely useful and according to me it already exist.

My point of view it's just mine, let's wait for other people to provide feedback on your implementation

@samber Samuel maybe

@samber
Copy link
Owner

samber commented Aug 18, 2024

IMO, it isn't the job of this library.

You can implement it in many ways:

mo.TupleToResult(io.Open(...)).
    MapErr(func (err error) {
          return nil, fmt.Errorf("an error: %w", err)
    })

or

f, err := io.Open(...)
if err != nil {
   return mo.Err(fmt.Errorf("an error: %w", err))
}

And if you chain multiple Map(), you cannot control what error you are wrapping.

FYI, in Go, we recommend contextualizing with a prefix followed by ":", instead of a suffix -> fmt.Errorf("an error: %w", err).

Side note and self-promotion: I made a one-liner library recently, that builds proper errors with stacktrace: https://github.com/samber/oops . It's not the only error wrapper available. See: https://github.com/avelino/awesome-go?tab=readme-ov-file#error-handling

@knpwrs
Copy link
Author

knpwrs commented Aug 19, 2024

I was wondering what it would look like with Errorf. Thank you for that example, @samber! I hadn’t realized that you can return a tuple from MapErr.

@knpwrs knpwrs closed this Aug 19, 2024
@knpwrs
Copy link
Author

knpwrs commented Aug 19, 2024

Also, oops looks really nice. Thank you again @samber!

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.

3 participants