search

pb33f / libopenapi

libopenapi is a fully featured, high performance OpenAPI 3.1, 3.0 and Swagger parser, library, validator and toolkit for golang applications.
https://pb33f.io/libopenapi/
Other
425 stars 56 forks source link

"Warning" error design improvement #155

Open jxsl13 opened 1 year ago

jxsl13 commented 1 year ago

Hi,

I read through the quick introduction to this library and found the error handling quite weird.

The current behavior is that you get a list of errors back for real errors and for warnings. Warnings being this special error for cyclic references.

Imo when at some point another such warning is introduced or somebody wants to introduce it, then it will not be possible without breacking backwards compatibility or breaking tools relying on this library by returning an additional warning error (or using this exact pattern below or having a big config struct as is seemingly the case in this library).

I would suggest not to return warnings as errors unless configured to do so by the library user. Configiration might be done by using the options patterns which utilizes variadic args.

Because currently the error is already returned and backwards compatibility might break otherwise, it might make sense to set the cyclic error default to true but allow to disable it with an option.

Example of the options pattern:

Default values are directly visible to the library user: https://github.com/jxsl13/amqpx/blob/main/pool/connection.go#L50-L63

https://github.com/jxsl13/amqpx/blob/main/pool/connection_options.go#L11

The returned error list is also pretty non-idiomatic but that's for maybe another discussion (errors.Join, errors.Is, errors.As and fmt.Errorf with multiple %w (introduced not too long ago,tho) might be a solution to that one)

117

daveshanley commented 1 year ago

Good suggestions. The only concern at this point is existing signatures all change for top-level APIs, which means this needs to be a minor release.