Set the default value for a pointers (non-struct) when it is nil only. #24
+30
−2
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
creasty
reviewed
May 29, 2021
Comment on lines
+62
to
+64
| defaultValueSet := false | ||
| if isInitialValue(field) { | ||
| defaultValueSet = true |
There was a problem hiding this comment.
This should be more sophisticated.
Suggested change
| defaultValueSet := false | |
| if isInitialValue(field) { | |
| defaultValueSet = true | |
| isInitial := isInitialValue(field) | |
| if isInitial { |
| String string `default:"hello"` | ||
| Duration time.Duration `default:"10s"` | ||
|
|
||
| IntOct int `default:"0o1"` | ||
| IntOctPtr *int `default:"0o1"` |
There was a problem hiding this comment.
Since its default value doesn't need to be an octal integer notation, we would like to add a more generic field like IntPtr instead.
Int int `default:"1"`
+ IntPtr *int `default:"1"`
...
IntOct int `default:"0o1"`
- IntOctPtr *int `default:"0o1"`Also, I'd appreciate it if could you add test cases for pointers of other primitive types (namely *unit, *float32, and *string) and those of a slice (*[]int) and a map (*map[string]int).
|
I also need this functionality. |
|
I'm taking over |
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Usually, in our projects, we read a JSON configuration file into a struct and invoke the defaults.Set() function to set the default values of non initialized fields. When one of the configuration parameters is a boolean and we set its default value to true, it cannot use the JSON configuration file to set it as false, because Set() assumes it using the default value and modifies it to true. In order to solve this, we decided to use pointers to boolean that can be nil when the field does not appear in the configuration file. In such a scenario, defaults.Set() works fine as expected assigning true to the boolean field, however, when in the JSON configuration file the boolean parameter is set to false, then defaults.Set() ignores that the pointer already has a value different from nil, gets the pointer value, checks that it is false, and modifies it to true what is not desired.
As a proposal, this PR just skips the setFields() function when the pointer is not nil and not a struct. Using this approach, the nil value will be the trigger to set the default value which makes sense since the Zero() value for pointers is nil.