Always call decoders

[sethvargo]

This reverts part of #62 to always process decoder interfaces for struct fields. This removes a breaking change where certain built-in types like net/url.URL have a custom unmarshaling function for the empty string.

The other rules for "overwrite" still apply.

Part of #64

[sethvargo]

@williamgcampbell @twmb PTAL at this and let me know what you think. I added tests specifically from #61 to show that the Zap use case is still preserved even after reverting this part of 62.

[sethvargo]

This is the main behavior change that is reverted from #62.

[twmb]

I'm not sure why a struct field should be treated differently from a typed non-struct field -- this preserves the zapcore.Level case, but only inadvertently. If zapcore.Level were defined as

type Level struct {
    v uint8
}

then the problem would emerge. From a consistency perspective, I'd expect decoders to either always be called, or never be called -- this seems to special case structs. In may case, I'm not adding ,default= to fields. For an example of if zapcore structured their code a bit differently:

diff --git a/envconfig_test.go b/envconfig_test.go
index 015250e..aff07d5 100644
--- a/envconfig_test.go
+++ b/envconfig_test.go
@@ -43,12 +43,14 @@ func (c *CustomDecoderType) EnvDecode(val string) error {
 }

 // Level mirrors Zap's level marshalling to reproduce an issue for tests.
-type Level int8
+type Level struct {
+       V int8
+}

-const (
-       DebugLevel Level = 0
-       InfoLevel  Level = 5
-       ErrorLevel Level = 100
+var (
+       DebugLevel Level = Level{0}
+       InfoLevel  Level = Level{5}
+       ErrorLevel Level = Level{100}
 )

 func (l *Level) UnmarshalText(text []byte) error {
@@ -2153,16 +2155,16 @@ func TestProcessWith(t *testing.T) {
                },
                {
                        // https://github.com/sethvargo/go-envconfig/issues/61
-                       name: "custom_decoder_overwrite_existing_value",
+                       name: "custom_decoder_not_overwrite_existing_value",
                        input: &struct {
-                               Level Level `env:"LEVEL,overwrite,default=error"`
+                               Level Level `env:"LEVEL,overwrite"`
                        }{
-                               Level: InfoLevel,
+                               Level: DebugLevel,
                        },
                        exp: &struct {
-                               Level Level `env:"LEVEL,overwrite,default=error"`
+                               Level Level `env:"LEVEL,overwrite"`
                        }{
-                               Level: InfoLevel,
+                               Level: DebugLevel,
                        },
                        lookuper: MapLookuper(nil),
                },

I get the failure:

$ go test
--- FAIL: TestProcessWith (0.00s)
    --- FAIL: TestProcessWith/custom_decoder_not_overwrite_existing_value (0.00s)
        envconfig_test.go:2272: mismatch (-want, +got):
              &struct{ Level envconfig.Level "env:\"LEVEL,overwrite\"" }{
            - 	Level: envconfig.Level{},
            + 	Level: envconfig.Level{V: 5},
              }
FAIL
exit status 1
FAIL	github.com/sethvargo/go-envconfig	0.280s

I think the main fix for #64 was #67 to propagate noinit. This reverts part of #62 so fix the breakage between 0.7.0 and 0.8.0, but I do still hold the opinion that non-existent env vars should not call custom decoders with empty values. This also makes it impossible to differentiate between an unset env var and an env var set to an empty string.

[sethvargo]

I do still hold the opinion that non-existent env vars should not call custom decoders with empty values.

I think I agree with you, but unfortunately users are depending on that current behavior and I don't want to introduce that subtly breaking change in a minor release. I've opened #69 to track changing this in the 1.0 release.

[twmb]

I agree from the angle of the Hyrum's Law aspect, but another angle to it is that by keeping the current state, then more people will be dependent on the current behavior by v1, and this may make it even less compelling to change the behavior for v1. Changing the behavior now could be framed as a bugfix ;)