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.

Sign up for GitHub

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

Jump to bottom

EncoderConfig and AtomicLevel validation added #791

Merged
merged 9 commits into from Mar 4, 2020
Merged

EncoderConfig and AtomicLevel validation added #791

Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
0385f6c
EncoderConfig and AtomicLevel validaton added
YashishDua Feb 21, 2020
65887ac
config.go: Drop unnecessary parentheses
abhinav Feb 22, 2020
dbd4283
Return err instead default adding default values
YashishDua Mar 1, 2020
09c30d5
Merge branch 'config-validation' of https://github.com/YashishDua/zap…
YashishDua Mar 1, 2020
7744f0b
test: Fix testify ordering
abhinav Mar 4, 2020
70c2b47
test: Verify error is non-nil
abhinav Mar 4, 2020
1e6b55b
test: Remove underscores from test names
abhinav Mar 4, 2020
3f647e8
Fix syntax error
abhinav Mar 4, 2020
dffa12d
encoder: Validate EnxoderConfig in newEncoder
abhinav Mar 4, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
14 changes: 14 additions & 0 deletions config.go
View file
Open in desktop
Expand Up @@ -174,6 +174,13 @@ func (cfg Config) Build(opts ...Option) (*Logger, error) {
return nil, err
}

if cfg.Level == (AtomicLevel{}) {
abhinav marked this conversation as resolved.
Show resolved Hide resolved
cfg.Level = NewAtomicLevelAt(InfoLevel)
if cfg.Development {
cfg.Level = NewAtomicLevelAt(DebugLevel)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we return an error instead?

If we prefer to add a default behavior for Level when unspecified, we'd probably want to document it on Config.Level.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I could see InfoLevel as default in ProductionConfig and DebugLevel in DevelopmentConfig, that's why thought of adding the same defaults here.

Let's return an error then!

}
}

log := New(
zapcore.NewCore(enc, sink, cfg.Level),
cfg.buildOptions(errSink)...,
Expand Down Expand Up @@ -239,5 +246,12 @@ func (cfg Config) openSinks() (zapcore.WriteSyncer, zapcore.WriteSyncer, error)
}

func (cfg Config) buildEncoder() (zapcore.Encoder, error) {
if cfg.EncoderConfig.TimeKey != "" && cfg.EncoderConfig.EncodeTime == nil {
Copy link
Collaborator

Choose a reason for hiding this comment

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

It seems that newEncoder is the ultimate constructor for zapcore.Encoder given
an EncoderConfig. We should maybe move this check to the newEncoder function,
leaving buildEncoder unchanged.

cfg.EncoderConfig.EncodeTime = zapcore.EpochTimeEncoder
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same here. It's not obvious why we pick epoch for production and ISO8601 for development. Perhaps an error would be preferable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

NewDevelopmentEncoderConfig specifies ISO8601TimeEncoder by default. I have not made any explicit choices here. But, let's return an error.

if cfg.Development {
cfg.EncoderConfig.EncodeTime = zapcore.ISO8601TimeEncoder
}
}

return newEncoder(cfg.Encoding, cfg.EncoderConfig)
}