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

"Warning" error design improvement #155

Open
jxsl13 opened this issue Aug 19, 2023 · 1 comment
Open

"Warning" error design improvement #155

jxsl13 opened this issue Aug 19, 2023 · 1 comment
Labels
enhancement New feature or request

Comments

@jxsl13
Copy link

jxsl13 commented Aug 19, 2023

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
Copy link
Member

daveshanley commented Aug 20, 2023

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.

@daveshanley daveshanley added the enhancement New feature or request label Oct 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants