Proposal: Default values already set in the passed struct pointer should be respected
- Dominant language
- Go
- Stars
- 6.3k
- Forks
- 284
- Avg merge
- 1d 17h
- Merged PRs (30d)
- 1
Description
Hey
Currently, doing something like this:
```go
type config struct {
Field string `env:"field,required,notEmpty"`
}
func New() *config {
return &config{
Field: FunctionThatComputesFieldDefault(),
}
}
func (c *config) Load() error {
// Error indicating that "Field" isn't set
return Parse(c)
}
```
Will result in error, as config.Field could not be found while parsing the environment, even though it had been set way before that.
Therefore, I propose respecting the pre-existing struct values when checking for `required` and `notEmpty`.
This should not be a breaking change, as it basically introduces a new feature that wasn't possible before. The upside of this, as shown in the example, is that you can compute a default, instead of just passing literals, but even just passing literals makes the whole thing much more readable than having everything in some magic string.
We would be willing to be the ones to implement this, but we wanted to check whether you'd want to accept this change at all. Alternatively, we could also make this an option, such as `RespectPreExistingStructValues` or something like that.
The change would be something along the lines of this:
```diff
diff --git a/env.go b/env.go
index be8ee53..04a7a7e 100644
--- a/env.go
+++ b/env.go
@@ -198,6 +198,10 @@ func doParse(ref reflect.Value, opts Options) error {
agrErr.Errors = append(agrErr.Errors, err)
}
}
+
+ if err := validateField(refField, refTypeField); err != nil {
+ agrErr.Errors = append(agrErr.Errors, err)
+ }
}
if len(agrErr.Errors) == 0 {
@@ -207,6 +211,11 @@ func doParse(ref reflect.Value, opts Options) error {
return agrErr
}
+func validateField(refField reflect.Value, refTypeField reflect.StructField) error {
+ // Check for emptiness and requiredness
+ return nil
+}
+
func doParseField(refField reflect.Value, refTypeField reflect.StructField, opts Options) error {
if !refField.CanSet() {
return nil
@@ -282,7 +291,7 @@ func get(field reflect.StructField, opts Options) (val string, err error) {
prefix := opts.Prefix
key := prefix + ownKey
defaultValue, defExists := field.Tag.Lookup("envDefault")
- val, exists, isDefault = getOr(key, defaultValue, defExists, opts.Environment)
+ val, exists, isDefault = getOr( /* Pass exsting field reference to get `isDefault`*/ key, defaultValue, defExists, opts.Environment)
if expand {
val = os.ExpandEnv(val)
@@ -292,14 +301,6 @@ func get(field reflect.StructField, opts Options) (val string, err error) {
defer os.Unsetenv(key)
}
- if required && !exists && len(ownKey) > 0 {
- return "", newEnvVarIsNotSet(key)
- }
-
- if notEmpty && val == "" {
- return "", newEmptyEnvVarError(key)
- }
-
if loadFile && val != "" {
filename := val
val, err = getFromFile(filename)
```
Contributor guide
No contributing guide indexed for this repository
Research direction
Start in env.go by reading doParse, doParseField, and get to understand where environment lookup and required/notEmpty validation currently occur. Define how pre-existing struct values affect those checks, then add coverage showing the proposed behavior and run the project test suite.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- go
- Domain
- backend
- Issue type
- Feature
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Stale
- Clarity
- Mostly clear
- Newbie friendliness
- 35/100