zapcore: Add ParseLevel

[Techassi]

This adds the public ParseLevel function which enables the user to
construct a log level based on ASCII text input.

This re-uses the UnmarshalText method of Level for the heavy-lifting,
but it avoids the user having to declare a local Level variable first.

var level zapcore.Level
err := level.UnmarshalText([]byte("debug"))

// versus
level, err := zapcore.ParseLevel("debug")

Usage

level, err := zapcore.LevelFromString("info") // zapcore.InfoLevel, nil

level, err := zapcore.LevelFromString("DEBUG") // zapcore.DebugLevel, nil

level, err := zapcore.LevelFromString("FOO") // -2, "invalid level"

One thing I'm not quite sure is the returned Level value of -2 when the user provided a invalid log level as text. Usually I would go for -1 but this is already used by zapcore.DebugLevel. Let me know if you know any better solution.

[CLAassistant]

All committers have signed the CLA.

[abhinav]

Hey there! Thanks for the contribution.

I'm not opposed to a convenience function, but I think:

1. It should be named UnmarshalLevel to clearly state that we're using the unmarshaling logic, which brings me to
2. It should not re-implement the unmarshaling logic. It should use the UnmarshalText method

[Techassi]

Makes sense. I already thought about that 👍

I will rework the implementation and will update this PR.

[Techassi]

I adjusted the implementation.

Adjusted Usage

level, err := zapcore.UnmarshalLevel("info") // zapcore.InfoLevel, nil

level, err := zapcore.UnmarshalLevel("DEBUG") // zapcore.DebugLevel, nil

level, err := zapcore.UnmarshalLevel("FOO") // zapcore.DebugLevel, "unrecognized level: FOO"

I was also thinking about adding a zap.NewAtomicLevelFrom(string) AtomicLevel convenience function to mirror the newly added UnmarshalLevel function. Let me know what you think about that.

[abhinav]

Could you please add a couple test cases for the new function in level_test.go? The cases you demonstrated in the comments should suffice.

RE: AtomicLevelFromString: Seems like it might be unnecessary since you can just call that on the result of UnmarshalLevel?

[codecov]

Codecov Report

Merging #1047 (b45b8a9) into master (ad0b02d) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #1047   +/-   ##
=======================================
  Coverage   98.21%   98.21%           
=======================================
  Files          48       48           
  Lines        2071     2074    +3     
=======================================
+ Hits         2034     2037    +3     
  Misses         29       29           
  Partials        8        8           

Impacted Files	Coverage Δ
zapcore/level.go	100.00% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ad0b02d...b45b8a9. Read the comment docs.

[abhinav]

Oh, re: this:

One thing I'm not quite sure is the returned Level value of -2 when the user provided a invalid log level as text. Usually I would go for -1 but this is already used by zapcore.DebugLevel. Let me know if you know any better solution.

The contract for ParseLevel, like most functions that return error as their last argument, is that if err != nil, the other value is invalid. So the user should not assume anything about the returned level if err != nil.

[Techassi]

I added the tests.

RE: AtomicLevelFromString: This seems like a similar convenience function as ParseLevel. The current usage is:

level := zap.NewAtomicLevel()
err := level.UnmarshalText(byte("debug"))

We could condense this into:

// Or zap.NewAtomicLevelFrom()
level := zap.NewAtomicLevelFromString("debug") // level = DebugLevel

RE this:

The contract for ParseLevel, like most functions that return error as their last argument, is that if err != nil, the other value is invalid. So the user should not assume anything about the returned level if err != nil.

Yeah this is usually how I handle this as well. But usually I set the first return argument to something which cleary states an error occurred. Returning 0 (or InfoLevel) in case of an error should be fine tho.

[abhinav]

RE: AtomicLevelFromString: This seems like a similar convenience function as ParseLevel. The current usage is:

level := zap.NewAtomicLevel()
err := level.UnmarshalText(byte("debug"))

We could condense this into:

// Or zap.NewAtomicLevelFrom()
level := zap.NewAtomicLevelFromString("debug") // level = DebugLevel

Fair enough. If you have need for it, a zap.ParseAtomicLevel would be fine.

[Techassi]

Great! I will add this asap.

PS: Thanks for the great input on the tests as this is the first time I work with tests in Go.

[abhinav]

Thanks!