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

Recommend errors.Is instead of == for testing sentinel errors? #2013

Closed
telemachus opened this issue Dec 21, 2021 · 5 comments
Closed

Recommend errors.Is instead of == for testing sentinel errors? #2013

telemachus opened this issue Dec 21, 2021 · 5 comments

Comments

@telemachus
Copy link
Contributor

The current introduction to Errors in Go recommends using == to test for sentinel errors, but I think that the community recommends using errors.Is instead.

For example, from the docs for errors:

Is unwraps its first argument sequentially looking for an error that matches the second. It reports whether it finds a match. It should be used in preference to simple equality checks:

if errors.Is(err, fs.ErrExist)

is preferable to

if err == fs.ErrExist

because the former will succeed if err wraps fs.ErrExist.

Would you be interested in a pull request that swapped errors.Is for ==? (Or perhaps one that mentions both—since people will see both in code—but recommends errors.Is?)

@jmrunkle
Copy link
Contributor

The difference in meaning is really subtle. We do not really go into error wrapping in the current exercise so I am not convinced we should change it without also introducing the concept of error wrapping. @junedev?

@junedev
Copy link
Member

junedev commented Dec 22, 2021

There were some discussions around this an the consens was that it is good to also know how to not unwrap so we are teaching this here. Also people need to learn about == since it is still used in a lot of code bases.

Since also covering error wrapping properly in this exercise would have been too much content, there will be a separate exercise for this, see issue #1997. @telemachus It would be great if you can leave the comment in that issue that we should explain that if there is no specific not to unwrap, errors.Is is preferred. Just to make sure we don't forget about it then.

@junedev junedev closed this as completed Dec 22, 2021
@telemachus
Copy link
Contributor Author

We do not really go into error wrapping in the current exercise so I am not convinced we should change it without also introducing the concept of error wrapping.

@jmrunkle Maybe I'm confused, but I'm not sure why this is necessary. Even if an error is not wrapped, doesn't errors.Is work? If that's so, can't this exercise recommend using errors.Is without mentioning wrapping?

To respond to @junedev's point ("people need to learn about == since it is still used in a lot of code bases"), I agree that == should be mentioned. How about this as a compromise?

You can compare error variables with the equality operator ==, and you will see this in older Go code. However, Go's documentation recommends using errors.Is for new code. (There could also be a sentence here explaining that errors.Is came into Go relatively recently with 1.13, though I don't think that detail is necessary.)

This lesson doesn't have to say anything about wrapping, and unless I'm confused all the examples should still work. (If not, I'd love to know since that means I'm wrong about errrors.Is.)

@junedev
Copy link
Member

junedev commented Dec 23, 2021

@telemachus errors.Is will work for non-wrapped errors like in the exercise but it will always unwrap for wrapped errors. I am not a fan of setting up this exercise in a way that it pretends errors.Is does nothing out of the ordinary because it is not true. I think it makes more sense to properly explain error wrapping first and then introduce errors.Is together with errors.As and explain the relationship of errors.Is and ==.

At work, we have several use cases were we specifically DON'T want to unwrap as it would lead to different behavior. E.g. we have an http error type that, if if would be returned up the chain would lead the server to respond with the enclosed status code. However, in some cases we don't want to forward that code so we wrap the error and then don't unwrap when checking it later, which in our case makes the server return 500 instead of whatever was in the original error. Similar for sentinels, e.g. ErrResourceNotFound in a repository/storage layer. In some context we know this had some other reason (e.g. it appeared in a place were something is really really wrong when the resource was not found at that point) and then we wrap it. In the layers above, we don't unwrap because the storage layer might know better and converted this error for a reason in some of the cases. We still want do be able to access the underlying error, e.g. in observability tools, that's why we don't convert to a string (e.g. use "%v" instead of "%w") but use wrapping.
Long story short, it is important to know and be able to use == which is why we make the student practice it here.

I would be happy to refer to the error wrapping concept right next to explaining == to prime the student that there is more to come but for that we need that other concept first.

@telemachus
Copy link
Contributor Author

I would be happy to refer to the error wrapping concept right next to explaining == to prime the student that there is more to come but for that we need that other concept first.

@junedev Thanks for the detailed explanation. I hadn't thought of cases where people would not want the unwrapping behavior, so that helps. I'll leave a note on #1997 to edit this exercise after that one is written.

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

No branches or pull requests

3 participants