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

Wrap error returned from MarshalCBOR in MarshalerError #486

Closed
fxamacker opened this issue Feb 5, 2024 · 3 comments
Closed

Wrap error returned from MarshalCBOR in MarshalerError #486

fxamacker opened this issue Feb 5, 2024 · 3 comments
Assignees
Milestone

Comments

@fxamacker
Copy link
Owner

Currently, error returned from MarshalCBOR() are returned from codec as is, without being wrapped.

PR #485 adds MarshalerError, which wraps error from well-formedness check for CBOR data item returned from MarshalCBOR().

However, error returned directly from MarshalCBOR() should also be wrapped in MarshalerError to be consistent with Go's encoding/json package.

Resolving this would be a breaking change due to different type of error being returned which only affects error handling of Marshaler.

@fxamacker fxamacker self-assigned this Feb 5, 2024
@fxamacker fxamacker added this to the v2.7.0 milestone Feb 12, 2024
@fxamacker
Copy link
Owner Author

Maybe this bugfix can be justified as part of larger PR #485 but I don't like changes that might be breaking changes for some.

@benluddy what are your thoughts on this specific bugfix and this type of bugfix in general? We currently follow SemVer with some limited exceptions noted in README.

@benluddy
Copy link
Contributor

benluddy commented Mar 4, 2024

I agree with the desired behavior but wouldn't make the breaking change. Users who differentiate error handling behaviors based on specific errors from a cbor.Marshaler are exactly the same users that would potentially be broken by wrapping the error, and other users would only see marginal benefit.

Instead of making this change and trying to communicate it loudly to users, WDYT about addressing this as part of a new marshaler interface (#485 (review))?

@fxamacker
Copy link
Owner Author

Instead of making this change and trying to communicate it loudly to users, WDYT about addressing this as part of a new marshaler interface (#485 (review))?

@benluddy Thanks, I opened issue to add new interface as alternative to Marshaler:

The new interface can address limitations of Marshaler discussed here and also discussed for proposed encoding/json/v2 at

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants