kelseyhightower / envconfig

Golang library for managing configuration data from environment variables
MIT License
5.02k stars 377 forks source link

Smart splitting of fields #91

Closed takama closed 7 years ago

takama commented 7 years ago

Sometimes ENV vars contain empty values. If we have an empty ENV, it should be transformed to an empty slice of strings, we should correct the solution to not to have []string{""} with len(s)==1.

value := ""
vals := strings.Split(value, ",")
if len(vals) != 0 {
    fmt.Pritnln("Empty ENV generates extra element")
}

Another case with extra spaces. Would be convenient to get

[]string{"unit1:1" "unit2" "unit3"}

from string " unit1:1, unit2 , unit3 ",

instead of

[]string{" unit1:1" " unit2 " " unit3 "}
teepark commented 7 years ago

Thanks for the PR @takama! These are good ideas and I'm totally on board with the test code. But the use of FieldFunc will also cause it to split strings with spaces into separate slice items, which isn't what we want. I think we'll need to just explicitly check for the empty string, and TrimSpace around each item instead.

takama commented 7 years ago

There is no problem to use

vals := strings.FieldsFunc(value, func(c rune) bool { return c == ','})

instead of

vals := strings.FieldsFunc(value, func(c rune) bool { return c == ',' || c == ' '})

Which get is correct result anyway https://play.golang.org/p/nCvjEOs3us

takama commented 7 years ago

What about https://play.golang.org/p/e5LDOFwWoS Each random comma will generate an extra element, any reasonable use case why we need that for maps? e.g. env MYAPP_MAP ",,,," for type Any struct{ Map map[string]string } will get envconfig.Process: assigning MYAPP_MAP to Map: converting ',,,,' to type map[string]string. details: invalid map item: ""

teepark commented 7 years ago

I was primarily concerned with the slice case, where this change makes it impossible to ever save an empty string. Suppose a user is assigning specific meaning to the first, second and third items of a slice, so they store an environment variable of "foo,,bar" with the clear intention of creating []string{"foo", "", "bar"} -- this change prevents that.

When it comes to maps though, ",,," is most definitely invalid, and the best thing to do is produce an error, not paper over the issue with an empty map.