zap icon indicating copy to clipboard operation
zap copied to clipboard

Feature Request: toml tags for configs

Open prepodavan opened this issue 4 years ago • 3 comments

can i hope to see this feauture anytime later? some fields of EncoderConfig have json+yaml tags that differs from field name (but have no toml tag). that forces me to use configs with different keys in json and toml. it impossible to use toml for zap.Config (with same key as in json) since without tags toml can't recognize that some fields of EncoderConfig is encoding.TextUnmarshler so they have nil and panics when logger tries to use it code for reproducing

package main

import (
	"fmt"
	"github.com/BurntSushi/toml"
	"go.uber.org/zap"
)

func main() {
	var cfg1 zap.Config
	_, err1 := toml.Decode(`
level = "debug"
encoding = "json"
outputPaths = ["stdout"]
errorOutputPaths = ["stderr"]

    [encoderConfig]
        messageKey = "message"
        levelKey = "level"
        encodeLevel = "lowercase"	# real field name
`, &cfg1)

	var cfg2 zap.Config
	_, err2 := toml.Decode(`
level = "debug"
encoding = "json"
outputPaths = ["stdout"]
errorOutputPaths = ["stderr"]

    [encoderConfig]
        messageKey = "message"
        levelKey = "level"
        levelEncoder = "lowercase" 		# json+yaml tag (provided by example of json config from doc)
`, &cfg2)

	fmt.Println(err1, err2) // nil nil
	l1, err1 := cfg1.Build()
	l2, err2 := cfg2.Build()
	fmt.Println(err1, err2) // nil nil
	l1.Debug("hello world") // its fine
	l2.Debug("hello world") // panic as long as cfg2.EncoderConfig.EncodeLevel is nil as long as
	// not recognized as encoding.TextUnmarshaler
}

prepodavan avatar Aug 03 '20 16:08 prepodavan

If this is something the maintainers are okay with considering, then I can put up a PR adding toml tags to the configs.

LeviMatus avatar Sep 27 '20 01:09 LeviMatus

We are fine with adding toml tags to zap, and taking on a toml test-only dependency. Please avoid adding toml dependencies to the non-test code.

prashantv avatar Sep 30 '20 23:09 prashantv

In my continued work on #868, looking at the most commonly used TOML parsing/encoding libraries (BurntSushi/toml, pelletier/go-toml, and naoina/toml), integration into the existing code isn't as simple as initially thought. The main issue is

[timeEncoder]
layout = "06/01/02 03:04pm"

and the type of zapcore.TimeEncoder, func(time.Time, PrimitiveArrayEncoder). In my testing, these packages don't play nicely with taking the timeEncoder config as a map value. Some throw errors and others panic. The behavior occurs even when extending the method-set of TimeEncoder to include UnmarshalTOML(interface{}) error. The libraries complain about trying to assign a map[string]interface{} in a func. I'd expect them to pickup on the UnmarshalTOML method and let it handle the logic, but they do not - at least not in all cases. If I add the UnmarshalTOML method to zapcore.EncoderConfig, the method does get called. But when attached to zapcore.TimeEncoder the method does not get called.

I'm not familiar with these TOML packages, so I could be missing something obvious. I suspect this won't integrate nicely with Zap, though.

LeviMatus avatar Oct 04 '20 02:10 LeviMatus