Skip to content

"Warning" error design improvement #155

@jxsl13

Description

@jxsl13

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

Metadata

Metadata

Assignees

No one assigned

    Labels

    enhancementNew feature or request

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions