|
|
|
Created:
11 years, 12 months ago by abel.deuring Modified:
11 years, 12 months ago Visibility:
Public. |
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)
Patch Set 1 #
Total comments: 3
Patch Set 2 : Better error message for invalid keys in config #Patch Set 3 : Better error message for invalid keys in config #
Total comments: 4
Patch Set 4 : Better error message for invalid keys in config #
Total comments: 6
Patch Set 5 : Better error message for invalid keys in config #
Total messages: 12
|
abel.deuring
Please take a look.
|
11 years, 12 months ago (2014年01月13日 14:07:16 UTC) #1 | |||||||||||||||||||||||||||||||
Please take a look.
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)"
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.
Please take a look.
Please take a look.
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 ...
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.
Please take a look.
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() }
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.