Better error message for invalid keys in config

Better error message for invalid keys in config

This is a fix for bug 1117173: Validating config could have better errors.

Using the common error formatting function error_() does not make sense
for invalid keys, I think, so just used fmt.Errorf().

https://code.launchpad.net/~adeuring/juju-core/1117173/+merge/201417

(do not edit description out of merge proposal)

[abel.deuring, 2014-01-13 14:07:16]

Please take a look.

[dimitern, 2014-01-13 14:14:23]

Looks OK, just a few suggestions.

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

https://codereview.appspot.com/51280045/diff/1/schema/schema.go#newcode30
schema/schema.go:30: func spath(path []string) string {

I'd prefer a more descriptive name for this helper, and ideally a doc comment,
because it's a bit hard to follow.

https://codereview.appspot.com/51280045/diff/1/schema/schema.go#newcode408
schema/schema.go:408: return nil, fmt.Errorf("%s: Unknown key %s", spath(path),
ks)

IMO it's better to use %q in these cases, instead of %s (both places).
Also, isn't it better to provide the value in the error message if possible? 

"%q: unknown key %q (value %q)"

[mue, 2014-01-13 15:00:27]

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

https://codereview.appspot.com/51280045/diff/1/schema/schema.go#newcode30
schema/schema.go:30: func spath(path []string) string {
On 2014/01/13 14:14:23, dimitern wrote:
> 
> I'd prefer a more descriptive name for this helper, and ideally a doc comment,
> because it's a bit hard to follow.

Yes, would like joindPath() or similar and a doc too.

[abel.deuring, 2014-01-14 12:24:28]

Please take a look.

[abel.deuring, 2014-01-14 12:29:05]

Please take a look.

[mue, 2014-01-14 12:56:43]

https://codereview.appspot.com/51280045/diff/40001/schema/schema.go
File schema/schema.go (right):

https://codereview.appspot.com/51280045/diff/40001/schema/schema.go#newcode32
schema/schema.go:32: func path_as_string(path []string) string {
Naming convention in Go is camel-case: pathAsString().

Also the comments start with the function/method name:

// pathAsString returns a string consisting of the path elements. If ...

https://codereview.appspot.com/51280045/diff/40001/schema/schema.go#newcode417
schema/schema.go:417: return nil, fmt.Errorf("%v: Unknown key %q (value %q)",
path_as_string(path), ks, value)
Better, only change to lower case: ...: unknown key ...

[abel.deuring, 2014-01-14 13:08:16]

https://codereview.appspot.com/51280045/diff/40001/schema/schema.go
File schema/schema.go (right):

https://codereview.appspot.com/51280045/diff/40001/schema/schema.go#newcode32
schema/schema.go:32: func path_as_string(path []string) string {
On 2014/01/14 12:56:44, mue wrote:
> Naming convention in Go is camel-case: pathAsString().
> 
> Also the comments start with the function/method name:
> 
> // pathAsString returns a string consisting of the path elements. If ...

right -- I'm still living in another world ;)

https://codereview.appspot.com/51280045/diff/40001/schema/schema.go#newcode417
schema/schema.go:417: return nil, fmt.Errorf("%v: Unknown key %q (value %q)",
path_as_string(path), ks, value)
On 2014/01/14 12:56:44, mue wrote:
> Better, only change to lower case: ...: unknown key ...

Done.

[abel.deuring, 2014-01-14 15:09:58]

Please take a look.

[mue, 2014-01-15 08:06:52]

https://codereview.appspot.com/51280045/diff/50001/schema/schema.go
File schema/schema.go (right):

https://codereview.appspot.com/51280045/diff/50001/schema/schema.go#newcode30
schema/schema.go:30: // Return a string consisting of the path elements. If path
starts
Still would like a standard Go comment here, beginning with the function name:

// pathAsString returns a string consisting ...

https://codereview.appspot.com/51280045/diff/50001/schema/schema.go#newcode41
schema/schema.go:41: path := pathAsString(e.path)
Is the function only used here? Then it could also be a method of error_.

Btw, I dislike the trailing underscore in error_. But that's not your problem.
;)

https://codereview.appspot.com/51280045/diff/50001/schema/schema.go#newcode410
schema/schema.go:410: var value interface{}
A bit shorter:

value := interface{}("(invalid)")
valuev := rv.MapIndex(k)
if valuev.IsValid()  {
    value = valuev.Interface()
}

[abel.deuring, 2014-01-15 10:50:28]

https://codereview.appspot.com/51280045/diff/50001/schema/schema.go
File schema/schema.go (right):

https://codereview.appspot.com/51280045/diff/50001/schema/schema.go#newcode30
schema/schema.go:30: // Return a string consisting of the path elements. If path
starts
On 2014/01/15 08:06:53, mue wrote:
> Still would like a standard Go comment here, beginning with the function name:
> 
> // pathAsString returns a string consisting ...

Fixed.

https://codereview.appspot.com/51280045/diff/50001/schema/schema.go#newcode41
schema/schema.go:41: path := pathAsString(e.path)
On 2014/01/15 08:06:53, mue wrote:
> Is the function only used here? Then it could also be a method of error_.
> 
> Btw, I dislike the trailing underscore in error_. But that's not your problem.
> ;)

No, pathAsString() is also used in func (c fieldMapC) Coerce(v interface{}, path
[]string)... The fact that the latter needed a error message different from the
default one generated by error_(), was the reason to add the function
pathAsString().

https://codereview.appspot.com/51280045/diff/50001/schema/schema.go#newcode410
schema/schema.go:410: var value interface{}
On 2014/01/15 08:06:53, mue wrote:
> A bit shorter:
> 
> value := interface{}("(invalid)")
> valuev := rv.MapIndex(k)
> if valuev.IsValid()  {
>     value = valuev.Interface()
> }

fixed.

[abel.deuring, 2014-01-15 13:18:31]

Please take a look.

[mue, 2014-01-15 13:19:44]

LGTM