Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

gen/zap: Record all errors #378

Merged
merged 1 commit into from
Aug 28, 2018
Merged

gen/zap: Record all errors #378

merged 1 commit into from
Aug 28, 2018

Conversation

abhinav
Copy link
Contributor

@abhinav abhinav commented Aug 27, 2018

When appending/adding an object or array to a Zap encoder, there's a
possibliity of failure. Previously, we were recording the first failure
and returning.

This changes the generated code to rely on multierr and collect all
such failures into a single error.

Resolves #367

when appending/adding an object or array to a Zap encoder, there's a
possibliity of failure. Previously, we were recording the first failure
and returning.

This changes the generated code to rely on `multierr` and collect all
such failures into a single error.

Resolves #367
@abhinav abhinav requested review from mh-park and kriskowal August 27, 2018 21:35
@abhinav abhinav mentioned this pull request Aug 27, 2018
11 tasks
Copy link
Contributor

@mh-park mh-park left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. What's the expected behavior for the case where we have a mix of success and errors?

@abhinav
Copy link
Contributor Author

abhinav commented Aug 27, 2018

Zap won't log the objects if they fail to marshal.

@abhinav abhinav requested a review from akshayjshah August 28, 2018 18:45
@abhinav
Copy link
Contributor Author

abhinav commented Aug 28, 2018

Going to merge and release this shortly if there are no objections.

Copy link

@akshayjshah akshayjshah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉 This is great. 🎉

Among all our packages, multierr.Append is starting to be my favorite API.

@akshayjshah
Copy link

Zap won't log the objects if they fail to marshal.

This isn't quite true. It's entirely possible for implementations of zapcore.ObjectMarshaler and zapcore.ArrayMarshaler to corrupt the output. All implementations of these methods should be doing any required validation before they write to the encoder.

Here, I think we're safe - everything is (eventually) primitive types, so none of these implementations can ever actually fail.

@abhinav abhinav merged commit ae73793 into dev Aug 28, 2018
@abhinav abhinav deleted the abg/zap-error-collate branch August 28, 2018 22:47
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants