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

Non standard return value of 'ok' in FromError() #180

Open
wiegell opened this issue Jun 6, 2024 · 4 comments
Open

Non standard return value of 'ok' in FromError() #180

wiegell opened this issue Jun 6, 2024 · 4 comments

Comments

@wiegell
Copy link

wiegell commented Jun 6, 2024

Normally you would expect ok to be true, if a type assertion is correct, see e.g.:
https://go.dev/tour/methods/15

However for the FromError method the return value is false, when the underlying type assertion is correct (from docs):
image
From source:
image

That's not intuitive and goes against what "ok" means in many other cases in go.
Changing this would ofc. be a breaking change

@YoEight
Copy link
Member

YoEight commented Jun 13, 2024

Hey @wiegell,

I believe there is a misconception on what the FromError method does. If true, the ok doesn't signal that the type conversion was successful but rather that there was no error.

FromError is used get you back an esdb.Error if the error originated from the library. If the error was due to something outside of the library scope, we still wrap that error into a esdb.Error but classify it as Unknown.

@YoEight YoEight closed this as completed Jul 3, 2024
@wiegell
Copy link
Author

wiegell commented Jul 3, 2024

Sorry i never replied. I saw that it's a pattern you inherited from the grpc package - right? Anyway i don't think thats intuitive either. In general i'm a fan of using errors.Is() and errors.As(), which we have adopted in our project from the uber style guide: https://github.com/uber-go/guide/blob/master/style.md#errors
It seems you are also encouraging that sometimes, e.g. on the io.EOF:
image
Then the fromError() would not be needed at all

@YoEight YoEight reopened this Jul 4, 2024
@YoEight
Copy link
Member

YoEight commented Jul 4, 2024

Seems like something we could provide as well. Lay out exactly what you need and I see what I can do

@wiegell
Copy link
Author

wiegell commented Jul 4, 2024

So many places errors are returned like this:
image
Which is fine according to the style guide, if no error matching is needed.

I made an example of what i think is better when error matching is needed. What will be hard is if you want backwards compatibility with the current code system. Clone them to adjecent folders, since the example depends on replace github.com/EventStore/EventStore-Client-Go/v4 => ../EventStore-Client-Go:
https://github.com/wiegell/EventStore-Client-Go
https://github.com/wiegell/evenstore-error-example

The changes i've made to eventstore:
wiegell@82bf235

IMO this way simplifies error matching. I would also encourage more error wrapping, since it can be confusing to get a raw grpc error from the esdb.Client, which happens sometimes. Uber also have some recommendations on wrapping:
https://github.com/uber-go/guide/blob/master/style.md#error-wrapping

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

2 participants