You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
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.
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
The text was updated successfully, but these errors were encountered:
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
The text was updated successfully, but these errors were encountered: