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

zapcore: Add ParseLevel #1047

Merged
merged 5 commits into from Jan 13, 2022
Merged

zapcore: Add ParseLevel #1047

merged 5 commits into from Jan 13, 2022

Conversation

Techassi
Copy link
Contributor

@Techassi Techassi commented Jan 13, 2022

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.

This adds the public LevelFromString func which enables the user to construct a log level based on ASCII text input
@CLAassistant
Copy link

CLAassistant commented Jan 13, 2022

CLA assistant check
All committers have signed the CLA.

Copy link
Contributor

@abhinav abhinav left a comment

Choose a reason for hiding this comment

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

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
Copy link
Contributor Author

Makes sense. I already thought about that 👍

I will rework the implementation and will update this PR.

@Techassi
Copy link
Contributor Author

Techassi commented Jan 13, 2022

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.

@Techassi Techassi changed the title Add LevelFromString func Add UnmarshalLevel func Jan 13, 2022
@Techassi Techassi requested a review from abhinav Jan 13, 2022
Copy link
Contributor

@abhinav abhinav left a comment

Choose a reason for hiding this comment

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

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?

zapcore/level.go Outdated Show resolved Hide resolved
zapcore/level.go Outdated Show resolved Hide resolved
zapcore/level.go Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Jan 13, 2022

Codecov Report

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

Impacted file tree graph

@@           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 abhinav changed the title Add UnmarshalLevel func zapcore: Add ParseLevel Jan 13, 2022
@abhinav
Copy link
Contributor

abhinav commented Jan 13, 2022

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
Copy link
Contributor Author

Techassi commented Jan 13, 2022

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.

- We can use backticks to avoid having to escape quotes
- Prefer to have the error and non-error paths branch
  so that we're not asserting the value of level in the error case
  (since that's specifically not part of the contract)
@abhinav
Copy link
Contributor

abhinav commented Jan 13, 2022

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
Copy link
Contributor Author

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 abhinav merged commit e751939 into uber-go:master Jan 13, 2022
7 checks passed
@abhinav
Copy link
Contributor

abhinav commented Jan 13, 2022

Thanks!

@Techassi Techassi deleted the level-from-string branch Jan 13, 2022
sywhang added a commit that referenced this pull request Jan 14, 2022
As discussed in #1047 this PR adds a zap.ParseAtomicLevel() func which enables the user to construct a log level based on ASCII text input.

Usage
```
level, err := zap.ParseAtomicLevel("info") // InfoLevel, nil
level, err := zap.ParseAtomicLevel("DEBUG") // DebugLevel, nil
level, err := zap.ParseAtomicLevel("FOO") // InfoLevel, "unrecognized level: "FOO""
```

Co-authored-by: Sung Yoon Whang <sungyoonwhang@gmail.com>

Co-authored-by: Sung Yoon Whang <sungyoonwhang@gmail.com>
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

3 participants