Allow strings as bool/int config values

Allow strings as bool/int config values

Fix the parsing of config values to allow suitable
string values to be used to set bool and int
attributes. eg "true" mean true, "42" means 42 etc.
This is needed to allow juju set-env to work for those
types of values.

https://code.launchpad.net/~wallyworld/juju-core/set-env-boolean/+merge/196819

(do not edit description out of merge proposal)

[wallyworld, 2013-11-27 05:24:45]

Please take a look.

[rog, 2013-11-27 09:00:07]

Looks good, thanks, with a few suggestions below.

https://codereview.appspot.com/33680043/diff/1/schema/schema.go
File schema/schema.go (right):

https://codereview.appspot.com/33680043/diff/1/schema/schema.go#newcode105
schema/schema.go:105: if v != nil {
I think I'd be inclined to do something like this:

func (c boolC) Coerce(v interface{}, path []string) (interface{}, error) {
    switch vv := reflect.TypeOf(v); vv.Kind() {
    case reflect.Bool:
        return vv.Bool(), nil
    case reflect.String:
        val, err := strconv.ParseBool(vv.String())
        if err == nil {
            return val, nil
        }
    }
    return nil, error{"bool", v, path}
}

the Sprintf seems unnecessary overkill.

https://codereview.appspot.com/33680043/diff/1/schema/schema.go#newcode123
schema/schema.go:123: if v != nil {
similar here

https://codereview.appspot.com/33680043/diff/1/schema/schema.go#newcode146
schema/schema.go:146: intValue, err := strconv.ParseInt(fmt.Sprintf("%v", v), 0,
64)
Again, no need to use Sprintf. And given that we allow float values, we should
probably allow float string values too.

switch vv := reflect.ValueOf(v); vv.Kind() {
case reflect.String:
   intValue, err := strconv.ParseInt(vv.String(), 0, 64)
   if err == nil { 
       return int(intValue), nil
   }
   floatValue, err := strconv.ParseFloat(vv.String(), 0, 64)
   if err = nil {
       return int(floatValue), nil
   }
case reflect.Int, reflect.Int8, etc etc

https://codereview.appspot.com/33680043/diff/1/schema/schema_test.go
File schema/schema_test.go (right):

https://codereview.appspot.com/33680043/diff/1/schema/schema_test.go#newcode92
schema/schema_test.go:92: c.Assert(err, gc.ErrorMatches, `<path>: expected bool,
got int\(42\)`)
could we have a string value that fails too here, please?

https://codereview.appspot.com/33680043/diff/1/schema/schema_test.go#newcode161
schema/schema_test.go:161: c.Assert(err, gc.ErrorMatches, "<path>: expected
number, got nothing")
ditto.
and a test for a string containing a float value.

[wallyworld, 2013-11-27 09:40:17]

https://codereview.appspot.com/33680043/diff/1/schema/schema.go
File schema/schema.go (right):

https://codereview.appspot.com/33680043/diff/1/schema/schema.go#newcode105
schema/schema.go:105: if v != nil {
On 2013/11/27 09:00:08, rog wrote:
> I think I'd be inclined to do something like this:
> 
> func (c boolC) Coerce(v interface{}, path []string) (interface{}, error) {
>     switch vv := reflect.TypeOf(v); vv.Kind() {
>     case reflect.Bool:
>         return vv.Bool(), nil
>     case reflect.String:
>         val, err := strconv.ParseBool(vv.String())
>         if err == nil {
>             return val, nil
>         }
>     }
>     return nil, error{"bool", v, path}
> }
> 
> the Sprintf seems unnecessary overkill.

Why all that logic when strconv.ParseBool() does what we need with fewer lines
of code required? I can't see a reason for the reflection etc.

[rog, 2013-11-27 10:14:39]

On 27 November 2013 09:40,  <ian.booth@canonical.com> wrote:
>
> https://codereview.appspot.com/33680043/diff/1/schema/schema.go
> File schema/schema.go (right):
>
> https://codereview.appspot.com/33680043/diff/1/schema/schema.go#newcode105
> schema/schema.go:105: if v != nil {
> On 2013/11/27 09:00:08, rog wrote:
>>
>> I think I'd be inclined to do something like this:
>
>
>> func (c boolC) Coerce(v interface{}, path []string) (interface{},
>
> error) {
>>
>>      switch vv := reflect.TypeOf(v); vv.Kind() {
>>      case reflect.Bool:
>>          return vv.Bool(), nil
>>      case reflect.String:
>>          val, err := strconv.ParseBool(vv.String())
>>          if err == nil {
>>              return val, nil
>>          }
>>      }
>>      return nil, error{"bool", v, path}
>> }
>
>
>> the Sprintf seems unnecessary overkill.
>
>
> Why all that logic when strconv.ParseBool() does what we need with fewer
> lines of code required? I can't see a reason for the reflection etc.

For a start, my suggested code is more efficient and does no
unnecessary allocation, which is a good property to have in
a low level part of the code like this.

Secondly, your code will accept any type at all, as long as Sprintf
happens to marshal it to something that looks like a bool.
I'd much prefer to keep things more limited here, so that we
know exactly what types we accept.

My suggested code might be slightly longer, but it's much more
explicit in its behaviour.

[fwereade, 2013-11-27 22:58:22]

On 2013/11/27 10:14:39, rog wrote:
> Secondly, your code will accept any type at all, as long as Sprintf
> happens to marshal it to something that looks like a bool.
> I'd much prefer to keep things more limited here, so that we
> know exactly what types we accept.

For me, that's the clincher. I think there's actually a pretty strong argument
to be made for maximally-forgiving checkers in the schema package, but I'd
prefer to keep the change as limited as possible given that we're rolling it
into 1.16 and I don't want unnecessary divergence.

FWIW, roger, I am very much unconvinced by the efficiency argument here when
viewed in the light of the opinions you expressed earlier today re rpc calls ;p.

So, with the reflect-based version, LGTM.

[wallyworld, 2013-11-28 03:55:05]

https://codereview.appspot.com/33680043/diff/1/schema/schema.go
File schema/schema.go (right):

https://codereview.appspot.com/33680043/diff/1/schema/schema.go#newcode105
schema/schema.go:105: if v != nil {
On 2013/11/27 09:00:08, rog wrote:
> I think I'd be inclined to do something like this:
> 
> func (c boolC) Coerce(v interface{}, path []string) (interface{}, error) {
>     switch vv := reflect.TypeOf(v); vv.Kind() {
>     case reflect.Bool:
>         return vv.Bool(), nil
>     case reflect.String:
>         val, err := strconv.ParseBool(vv.String())
>         if err == nil {
>             return val, nil
>         }
>     }
>     return nil, error{"bool", v, path}
> }
> 
> the Sprintf seems unnecessary overkill.

Done.

https://codereview.appspot.com/33680043/diff/1/schema/schema.go#newcode123
schema/schema.go:123: if v != nil {
On 2013/11/27 09:00:08, rog wrote:
> similar here

Done.