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

Use errors.As() #3542

Open
wants to merge 4 commits into
base: v3
Choose a base branch
from
Open

Use errors.As() #3542

wants to merge 4 commits into from

Conversation

tchssk
Copy link
Member

@tchssk tchssk commented Jun 27, 2024

No description provided.

@tchssk tchssk marked this pull request as ready for review June 27, 2024 14:22
Copy link
Member

@raphael raphael left a comment

Choose a reason for hiding this comment

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

Thank you this is great! There is one case where we may not need to use As.

@@ -134,7 +135,11 @@ func (c *DSLContext) Record(err *Error) {
if c.Errors == nil {
c.Errors = MultiError{err}
} else {
c.Errors = append(c.Errors.(MultiError), err)
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if in this case this is necessary? The type of the Errors field is controlled by Goa and guaranteed to be MultiError, see https://github.com/goadesign/goa/blob/v3/eval/context.go#L16

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, but it can be a different error type in the Go type system. For clarity, how about changing DSLContext.Errors to MultiError?

Copy link
Member

Choose a reason for hiding this comment

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

Yes making that change makes sense

Copy link
Member Author

Choose a reason for hiding this comment

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

eval/eval.go Outdated
@@ -71,14 +72,20 @@ func Execute(fn func(), def Expression) bool {
}
var startCount int
if Context.Errors != nil {
startCount = len(Context.Errors.(MultiError))
var e MultiError
Copy link
Member

Choose a reason for hiding this comment

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

Same here, not sure this is needed.

}
Context.Stack = append(Context.Stack, def)
fn()
Context.Stack = Context.Stack[:len(Context.Stack)-1]
var endCount int
if Context.Errors != nil {
endCount = len(Context.Errors.(MultiError))
Copy link
Member

Choose a reason for hiding this comment

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

Also probably not needed

e, ok := err.(*ServiceError)
if !ok {
var e *ServiceError
if !errors.As(err, &e) {
Copy link
Member

Choose a reason for hiding this comment

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

Should this use Is?

Copy link
Member Author

Choose a reason for hiding this comment

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

This should be As. The Go Blog says

https://go.dev/blog/go1.13-errorse

In the simplest case, the errors.Is function behaves like a comparison to a sentinel error, and the errors.As function behaves like a type assertion

I added test for asError(). It fails if you replace As with Is.

a2d4dc8

Copy link
Member

Choose a reason for hiding this comment

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

Makes sense, thanks for the clarification!

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.

None yet

2 participants