search

EventStore / EventStore-Client-Go

Go Client for Event Store version 20 and above.
Apache License 2.0
103 stars 25 forks source link

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

Open wiegell opened 2 months ago

wiegell commented 2 months ago

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 commented 2 months ago

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.

wiegell commented 1 month ago

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 commented 1 month ago

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

wiegell commented 1 month ago

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: https://github.com/wiegell/EventStore-Client-Go/commit/82bf235e6517cf123c351b6435302f040105e77f

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