refactor: add default true type #998
Conversation
fchikwekwe
changed the title
fchikwekwe
changed the title
config/validate.go
Outdated
| @@ -184,6 +184,15 @@ func validateDatatype(k string, v any, typ string) string { | |||
| if u.Scheme != "http" && u.Scheme != "https" { | |||
| return fmt.Sprintf("field %s (%v) must use an http or https scheme", k, v) | |||
| } | |||
| case "defaulttrue": | |||
| if !isString(v) { | |||
| return fmt.Sprintf("field %s must be of type DefaultTrue", k) | |||
There was a problem hiding this comment.
This error message wouldn't be generated unless the field was of type DefaultTrue. This validation is checking the value portion of the configuration, so you actually want to check if _, ok := v.(bool) to make sure the user is specifying either true or false for the value.
I'm not sure you even need the UnmarshalText part below. But to confirm, you should try a couple of different values in a configuration and run the validator to make sure it does the right thing.
There was a problem hiding this comment.
This value comes in as a string, so I used strconv.ParseBool instead.
There was a problem hiding this comment.
I don't think it matters very much but modern YAML parsers want true and false and nothing else, while ParseBool also accepts 1 and 0. The YAML parser we use also accepts things like yes/no and ON/OFF, but only when they're applied to a boolean value (because that was in YAML 1.1, which is now old, but they do it for backwards compatibility).
I like the stricter validation, frankly -- we don't need to support yes/no -- but it might be worth a quick check to see if the YAML parser can read 1/0 into a bool. If not, then our validator should be stricter and just check for true/false and t/f case-insensitively.
There was a problem hiding this comment.
According the docs, it does not seem that 1/0 are supported as bools in the yaml parser. I'll fix the validation to check for "true"/"false", "t"/"f".
There was a problem hiding this comment.
Its a little ugly, but I've updated it.
config/validate.go
Outdated
| case bool: | ||
| return "" | ||
| case string: | ||
| if val != "t" && val != "f" && val != "true" && val != "false" { |
There was a problem hiding this comment.
Please use strings.ToLower(val). Another way to do this if you prefer would be something like:
switch strings.ToLower(val) {
case "t", "true", "f", "false":
default:
// error
There was a problem hiding this comment.
Sure, the second switch statement looks better to me.
Which problem is this PR solving?
Short description of the changes
DefaultTruetype to use when specifying values with a default of booleantruethis improves some getter functions by abstracting away the*boolbehavior and allows booleantruebe a true default