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

Accept capital strings in Level.UnmarshalText #435

Merged
merged 5 commits into from
May 25, 2017

Conversation

jcorbin
Copy link
Contributor

@jcorbin jcorbin commented May 18, 2017

Makes it easier to parse zap's own log output (especially since capital level strings is our internal standard).

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.

Should we just strings.ToLower(string(text)) so that we accept debug Debug and DEBUG?

@jcorbin
Copy link
Contributor Author

jcorbin commented May 18, 2017

  • I'd prefer to avoid introducing an alloc as I'm using this on the hot path (external down sampler) and not just as part of startup config parse
  • while I understand the impulse to "Parse Them All! ™", the all-caps version actually is more privileged since it's one of the configurable formats that zap itself will produce
  • my real intention with this pr was: "if we can write it (and we do by default internally), we should parse it"

@akshayjshah
Copy link
Contributor

@jcorbin What do you think of pulling out the switch into a separate function and making two attempts at parsing - once with the original string, and once with the lowercased version if the first pass fails? That keeps the success case fast, but successfully parses a wider array of reasonably well-formed inputs.

Copy link
Contributor

@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.

Excellent.

@akshayjshah akshayjshah merged commit b33459c into master May 25, 2017
@jcorbin jcorbin deleted the rt_cap_level_strings branch June 5, 2017 17:21
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.

4 participants