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

Add MarshalText to AtomicLevel and FilterMessageSnippet to observer #413

Closed
wants to merge 1 commit into from

Conversation

ZymoticB
Copy link
Contributor

My team displays its yaml configs on HTML debug pages and thus requires that AtomicLevel support marshalling.

Additionally, we are currently transitioning from logrus style format logging to zap.SugaredLogger and use a testing strategy where we assert that a log message contains some list of snippets, rather than asserting on the entire formatted log message. This seems generally useful for users of the SugaredLogger.

Copy link
Collaborator

@prashantv prashantv left a comment

Choose a reason for hiding this comment

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

Can we break this up into separate PRs since the MarshalText and observer changes look unrelated.

Regarding MarshalText, I'm not sure why it is on the pointer. Since functions like Enabled are on the value receiver, I'd expect MarshalText to also be on the value receiver. UnmarshalText modifies the underlying value so it must be on a pointer receiver, but the other methods shouldn't need to use pointers.

The same issue exists with Level currently, I'll send out a separate PR to change it to value receiver where we can discuss this a little more.

@ZymoticB
Copy link
Contributor Author

It was definitely awkward working with the pointer receiver there, thanks for the PR.

I'll break this up.

@ZymoticB
Copy link
Contributor Author

I'm closing this so that the two parts can be merged from sane branch names in my fork.

@ZymoticB ZymoticB closed this Apr 21, 2017
@ZymoticB
Copy link
Contributor Author

Observer is here GH-415
AtomicLevel will be rebase once GH-414 is finalized

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

Successfully merging this pull request may close these issues.

None yet

2 participants