Conversation
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.
|
@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. |
| }), | ||
| errMsg: "broken", | ||
| }, | ||
| { |
There was a problem hiding this comment.
This is the main behavior change that is reverted from #62.
|
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 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: 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. |
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. |
|
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 ;) |
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