Merge lp:~niemeyer/pyjuju/go-schema into lp:pyjuju/go
- go-schema
- Merge into go
Status: | Merged | ||||
---|---|---|---|---|---|
Approved by: | Gustavo Niemeyer | ||||
Approved revision: | 2 | ||||
Merged at revision: | 2 | ||||
Proposed branch: | lp:~niemeyer/pyjuju/go-schema | ||||
Merge into: | lp:pyjuju/go | ||||
Diff against target: |
617 lines (+587/-7) 2 files modified
schema/schema.go (+355/-1) schema/schema_test.go (+232/-6) |
||||
To merge this branch: | bzr merge lp:~niemeyer/pyjuju/go-schema | ||||
Related bugs: |
|
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Jim Baker (community) | Approve | ||
William Reade (community) | Approve | ||
Juju Engineering | Pending | ||
Review via email: mp+72811@code.launchpad.net |
Commit message
Description of the change
Ported schema verification and coercion logic from Python. (!)
Gustavo Niemeyer (niemeyer) wrote : | # |
Thanks for the review William. Some feedback:
[0][1]
Yes, in your suggested model we have to shrink and expand the list on every iteration of the loop, while this is a simple assignment. The Python version works the same way.
[2]
Our strict long variable name policy has lead us to a position that is not entirely nice. I'm still very much for readability, and that's precisely the point in fact. Long names in tight contexts are doing more harm than good in our code base, leading to very long lines that often wrap and easily become difficult to follow.
I still think we should be careful about names, even more when these names have a wider scope, but I'd like to propose we take a different stanza in Go, and attempt to follow the practices and style used by the Go development team itself (in a similar way we use PEP8 in Python). Go also has the advantage of typing and static compilation, so it's easier to infer what something is, and harder to make mistakes.
Just for context, here is an example of long names getting in the way of readability:
@inlineCall
def test_machine_
"""Verify that a machine cannot be removed before being unassigned"""
unit_state = yield service_
yield unit_state.
ex = yield self.assertFailure(
yield unit_state.
topology = yield self.get_topology()
removed = yield self.machine_
topology = yield self.get_topology()
# can do this multiple times
removed = yield self.machine_
[3]
I agree with you on that. I just can't work on this right now, but let's try to go into that direction.
William Reade (fwereade) wrote : | # |
I'm not entirely convinced by [2] but I'm willing to participate in the experiment. I've filed lp:833906 for [3], and I'm happy to approve.
Jim Baker (jimbaker) wrote : | # |
+1 It's a nice Rosetta Stone for comparing Python and Go, including how the concept, its implementation, and tests should be idiomatically done in Go. Very nice indeed!
Jim Baker (jimbaker) wrote : | # |
The above should have been a "approved review"
Preview Diff
1 | === modified file 'schema/schema.go' |
2 | --- schema/schema.go 2011-08-23 15:10:42 +0000 |
3 | +++ schema/schema.go 2011-08-24 23:58:23 +0000 |
4 | @@ -1,3 +1,357 @@ |
5 | package schema |
6 | |
7 | -const Nothing = 0 |
8 | +import ( |
9 | + "fmt" |
10 | + "os" |
11 | + "reflect" |
12 | + "regexp" |
13 | + "strconv" |
14 | + "strings" |
15 | +) |
16 | + |
17 | +// The Coerce method of the Checker interface is called recursively when |
18 | +// v is being validated. If err is nil, newv is used as the new value |
19 | +// at the recursion point. If err is non-nil, v is taken as invalid and |
20 | +// may be either ignored or error out depending on where in the schema |
21 | +// checking process the error happened. Checkers like OneOf may continue |
22 | +// with an alternative, for instance. |
23 | +type Checker interface { |
24 | + Coerce(v interface{}, path []string) (newv interface{}, err os.Error) |
25 | +} |
26 | + |
27 | +type error struct { |
28 | + want string |
29 | + got interface{} |
30 | + path []string |
31 | +} |
32 | + |
33 | +func (e error) String() string { |
34 | + var path string |
35 | + if e.path[0] == "." { |
36 | + path = strings.Join(e.path[1:], "") |
37 | + } else { |
38 | + path = strings.Join(e.path, "") |
39 | + } |
40 | + if e.want == "" { |
41 | + return fmt.Sprintf("%s: unsupported value", path) |
42 | + } |
43 | + if e.got == nil { |
44 | + return fmt.Sprintf("%s: expected %s, got nothing", path, e.want) |
45 | + } |
46 | + return fmt.Sprintf("%s: expected %s, got %#v", path, e.want, e.got) |
47 | +} |
48 | + |
49 | +// Any returns a Checker that succeeds with any input value and |
50 | +// results in the value itself unprocessed. |
51 | +func Any() Checker { |
52 | + return anyC{} |
53 | +} |
54 | + |
55 | +type anyC struct{} |
56 | + |
57 | +func (c anyC) Coerce(v interface{}, path []string) (interface{}, os.Error) { |
58 | + return v, nil |
59 | +} |
60 | + |
61 | + |
62 | +// Const returns a Checker that only succeeds if the input matches |
63 | +// value exactly. The value is compared with reflect.DeepEqual. |
64 | +func Const(value interface{}) Checker { |
65 | + return constC{value} |
66 | +} |
67 | + |
68 | +type constC struct { |
69 | + value interface{} |
70 | +} |
71 | + |
72 | +func (c constC) Coerce(v interface{}, path []string) (interface{}, os.Error) { |
73 | + if reflect.DeepEqual(v, c.value) { |
74 | + return v, nil |
75 | + } |
76 | + return nil, error{fmt.Sprintf("%#v", c.value), v, path} |
77 | +} |
78 | + |
79 | +// OneOf returns a Checker that attempts to Coerce the value with each |
80 | +// of the provided checkers. The value returned by the first checker |
81 | +// that succeeds will be returned by the OneOf checker itself. If no |
82 | +// checker succeeds, OneOf will return an error on coercion. |
83 | +func OneOf(options ...Checker) Checker { |
84 | + return oneOfC{options} |
85 | +} |
86 | + |
87 | +type oneOfC struct { |
88 | + options []Checker |
89 | +} |
90 | + |
91 | +func (c oneOfC) Coerce(v interface{}, path []string) (interface{}, os.Error) { |
92 | + for _, o := range c.options { |
93 | + newv, err := o.Coerce(v, path) |
94 | + if err == nil { |
95 | + return newv, nil |
96 | + } |
97 | + } |
98 | + return nil, error{path: path} |
99 | +} |
100 | + |
101 | +// Bool returns a Checker that accepts boolean values only. |
102 | +func Bool() Checker { |
103 | + return boolC{} |
104 | +} |
105 | + |
106 | +type boolC struct{} |
107 | + |
108 | +func (c boolC) Coerce(v interface{}, path []string) (interface{}, os.Error) { |
109 | + if reflect.TypeOf(v).Kind() == reflect.Bool { |
110 | + return v, nil |
111 | + } |
112 | + return nil, error{"bool", v, path} |
113 | +} |
114 | + |
115 | +// Int returns a Checker that accepts any integer value, and returns |
116 | +// the same value typed as an int64. |
117 | +func Int() Checker { |
118 | + return intC{} |
119 | +} |
120 | + |
121 | +type intC struct{} |
122 | + |
123 | +func (c intC) Coerce(v interface{}, path []string) (interface{}, os.Error) { |
124 | + switch reflect.TypeOf(v).Kind() { |
125 | + case reflect.Int: |
126 | + case reflect.Int8: |
127 | + case reflect.Int16: |
128 | + case reflect.Int32: |
129 | + case reflect.Int64: |
130 | + default: |
131 | + return nil, error{"int", v, path} |
132 | + } |
133 | + return reflect.ValueOf(v).Int(), nil |
134 | +} |
135 | + |
136 | +// Int returns a Checker that accepts any float value, and returns |
137 | +// the same value typed as a float64. |
138 | +func Float() Checker { |
139 | + return floatC{} |
140 | +} |
141 | + |
142 | +type floatC struct{} |
143 | + |
144 | +func (c floatC) Coerce(v interface{}, path []string) (interface{}, os.Error) { |
145 | + switch reflect.TypeOf(v).Kind() { |
146 | + case reflect.Float32: |
147 | + case reflect.Float64: |
148 | + default: |
149 | + return nil, error{"float", v, path} |
150 | + } |
151 | + return reflect.ValueOf(v).Float(), nil |
152 | +} |
153 | + |
154 | + |
155 | +// String returns a Checker that accepts a string value only and returns |
156 | +// it unprocessed. |
157 | +func String() Checker { |
158 | + return stringC{} |
159 | +} |
160 | + |
161 | +type stringC struct{} |
162 | + |
163 | +func (c stringC) Coerce(v interface{}, path []string) (interface{}, os.Error) { |
164 | + if reflect.TypeOf(v).Kind() == reflect.String { |
165 | + return reflect.ValueOf(v).String(), nil |
166 | + } |
167 | + return nil, error{"string", v, path} |
168 | +} |
169 | + |
170 | +func SimpleRegexp() Checker { |
171 | + return sregexpC{} |
172 | +} |
173 | + |
174 | +type sregexpC struct{} |
175 | + |
176 | +func (c sregexpC) Coerce(v interface{}, path []string) (interface{}, os.Error) { |
177 | + // XXX The regexp package happens to be extremely simple right now. |
178 | + // Once exp/regexp goes mainstream, we'll have to update this |
179 | + // logic to use a more widely accepted regexp subset. |
180 | + if reflect.TypeOf(v).Kind() == reflect.String { |
181 | + s := reflect.ValueOf(v).String() |
182 | + _, err := regexp.Compile(s) |
183 | + if err != nil { |
184 | + return nil, error{"valid regexp", s, path} |
185 | + } |
186 | + return v, nil |
187 | + } |
188 | + return nil, error{"regexp string", v, path} |
189 | +} |
190 | + |
191 | +// String returns a Checker that accepts a slice value with values |
192 | +// that are processed with the elem checker. If any element of the |
193 | +// provided slice value fails to be processed, processing will stop |
194 | +// and return with the obtained error. |
195 | +func List(elem Checker) Checker { |
196 | + return listC{elem} |
197 | +} |
198 | + |
199 | +type listC struct { |
200 | + elem Checker |
201 | +} |
202 | + |
203 | +func (c listC) Coerce(v interface{}, path []string) (interface{}, os.Error) { |
204 | + rv := reflect.ValueOf(v) |
205 | + if rv.Kind() != reflect.Slice { |
206 | + return nil, error{"list", v, path} |
207 | + } |
208 | + |
209 | + path = append(path, "[", "?", "]") |
210 | + |
211 | + l := rv.Len() |
212 | + out := make([]interface{}, 0, l) |
213 | + for i := 0; i != l; i++ { |
214 | + path[len(path)-2] = strconv.Itoa(i) |
215 | + elem, err := c.elem.Coerce(rv.Index(i).Interface(), path) |
216 | + if err != nil { |
217 | + return nil, err |
218 | + } |
219 | + out = append(out, elem) |
220 | + } |
221 | + return out, nil |
222 | +} |
223 | + |
224 | +// Map returns a Checker that accepts a map value. Every key and value |
225 | +// in the map are processed with the respective checker, and if any |
226 | +// value fails to be coerced, processing stops and returns with the |
227 | +// underlying error. |
228 | +func Map(key Checker, value Checker) Checker { |
229 | + return mapC{key, value} |
230 | +} |
231 | + |
232 | +type mapC struct { |
233 | + key Checker |
234 | + value Checker |
235 | +} |
236 | + |
237 | +func (c mapC) Coerce(v interface{}, path []string) (interface{}, os.Error) { |
238 | + rv := reflect.ValueOf(v) |
239 | + if rv.Kind() != reflect.Map { |
240 | + return nil, error{"map", v, path} |
241 | + } |
242 | + |
243 | + vpath := append(path, ".", "?") |
244 | + |
245 | + l := rv.Len() |
246 | + out := make(map[interface{}]interface{}, l) |
247 | + keys := rv.MapKeys() |
248 | + for i := 0; i != l; i++ { |
249 | + k := keys[i] |
250 | + newk, err := c.key.Coerce(k.Interface(), path) |
251 | + if err != nil { |
252 | + return nil, err |
253 | + } |
254 | + vpath[len(vpath)-1] = fmt.Sprint(k.Interface()) |
255 | + newv, err := c.value.Coerce(rv.MapIndex(k).Interface(), vpath) |
256 | + if err != nil { |
257 | + return nil, err |
258 | + } |
259 | + out[newk] = newv |
260 | + } |
261 | + return out, nil |
262 | +} |
263 | + |
264 | +type Fields map[string]Checker |
265 | +type Optional []string |
266 | + |
267 | +// FieldMap returns a Checker that accepts a map value with defined |
268 | +// string keys. Every key has an independent checker associated, |
269 | +// and processing will only succeed if all the values succeed |
270 | +// individually. If a field fails to be processed, processing stops |
271 | +// and returns with the underlying error. |
272 | +func FieldMap(fields Fields, optional Optional) Checker { |
273 | + return fieldMapC{fields, optional} |
274 | +} |
275 | + |
276 | +type fieldMapC struct { |
277 | + fields Fields |
278 | + optional []string |
279 | +} |
280 | + |
281 | +func (c fieldMapC) isOptional(key string) bool { |
282 | + for _, k := range c.optional { |
283 | + if k == key { |
284 | + return true |
285 | + } |
286 | + } |
287 | + return false |
288 | +} |
289 | + |
290 | +func (c fieldMapC) Coerce(v interface{}, path []string) (interface{}, os.Error) { |
291 | + rv := reflect.ValueOf(v) |
292 | + if rv.Kind() != reflect.Map { |
293 | + return nil, error{"map", v, path} |
294 | + } |
295 | + |
296 | + vpath := append(path, ".", "?") |
297 | + |
298 | + l := rv.Len() |
299 | + out := make(map[string]interface{}, l) |
300 | + for k, checker := range c.fields { |
301 | + vpath[len(vpath)-1] = k |
302 | + var value interface{} |
303 | + valuev := rv.MapIndex(reflect.ValueOf(k)) |
304 | + if valuev.IsValid() { |
305 | + value = valuev.Interface() |
306 | + } else if c.isOptional(k) { |
307 | + continue |
308 | + } |
309 | + newv, err := checker.Coerce(value, vpath) |
310 | + if err != nil { |
311 | + return nil, err |
312 | + } |
313 | + out[k] = newv |
314 | + } |
315 | + return out, nil |
316 | +} |
317 | + |
318 | +// FieldMapSet returns a Checker that accepts a map value checked |
319 | +// against one of several FieldMap checkers. The actual checker |
320 | +// used is the first one whose checker associated with the selector |
321 | +// field processes the map correctly. If no checker processes |
322 | +// the selector value correctly, an error is returned. |
323 | +func FieldMapSet(selector string, maps []Checker) Checker { |
324 | + fmaps := make([]fieldMapC, len(maps)) |
325 | + for i, m := range maps { |
326 | + if fmap, ok := m.(fieldMapC); ok { |
327 | + if checker, _ := fmap.fields[selector]; checker == nil { |
328 | + panic("FieldMapSet has a FieldMap with a missing selector") |
329 | + } |
330 | + fmaps[i] = fmap |
331 | + } else { |
332 | + panic("FieldMapSet got a non-FieldMap checker") |
333 | + } |
334 | + } |
335 | + return mapSetC{selector, fmaps} |
336 | +} |
337 | + |
338 | +type mapSetC struct { |
339 | + selector string |
340 | + fmaps []fieldMapC |
341 | +} |
342 | + |
343 | +func (c mapSetC) Coerce(v interface{}, path []string) (interface{}, os.Error) { |
344 | + rv := reflect.ValueOf(v) |
345 | + if rv.Kind() != reflect.Map { |
346 | + return nil, error{"map", v, path} |
347 | + } |
348 | + |
349 | + var selector interface{} |
350 | + selectorv := rv.MapIndex(reflect.ValueOf(c.selector)) |
351 | + if selectorv.IsValid() { |
352 | + selector = selectorv.Interface() |
353 | + for _, fmap := range c.fmaps { |
354 | + _, err := fmap.fields[c.selector].Coerce(selector, path) |
355 | + if err != nil { |
356 | + continue |
357 | + } |
358 | + return fmap.Coerce(v, path) |
359 | + } |
360 | + } |
361 | + return nil, error{"supported selector", selector, append(path, ".", c.selector)} |
362 | +} |
363 | |
364 | === modified file 'schema/schema_test.go' |
365 | --- schema/schema_test.go 2011-08-23 15:10:42 +0000 |
366 | +++ schema/schema_test.go 2011-08-24 23:58:23 +0000 |
367 | @@ -2,18 +2,244 @@ |
368 | |
369 | import ( |
370 | "testing" |
371 | - "launchpad.net/gocheck" |
372 | + . "launchpad.net/gocheck" |
373 | "launchpad.net/ensemble/go/schema" |
374 | + "os" |
375 | ) |
376 | |
377 | func Test(t *testing.T) { |
378 | - gocheck.TestingT(t) |
379 | + TestingT(t) |
380 | } |
381 | |
382 | type S struct{} |
383 | |
384 | -var _ = gocheck.Suite(S{}) |
385 | - |
386 | -func (s *S) TestEmpty(c *gocheck.C) { |
387 | - _ = schema.Nothing |
388 | +var _ = Suite(&S{}) |
389 | + |
390 | +type Dummy struct{} |
391 | + |
392 | +func (d *Dummy) Coerce(value interface{}, path []string) (coerced interface{}, err os.Error) { |
393 | + return "i-am-dummy", nil |
394 | +} |
395 | + |
396 | +var aPath = []string{"<pa", "th>"} |
397 | + |
398 | +func (s *S) TestConst(c *C) { |
399 | + sch := schema.Const("foo") |
400 | + |
401 | + out, err := sch.Coerce("foo", aPath) |
402 | + c.Assert(err, IsNil) |
403 | + c.Assert(out, Equals, "foo") |
404 | + |
405 | + out, err = sch.Coerce(42, aPath) |
406 | + c.Assert(out, IsNil) |
407 | + c.Assert(err, Matches, `<path>: expected "foo", got 42`) |
408 | +} |
409 | + |
410 | +func (s *S) TestAny(c *C) { |
411 | + sch := schema.Any() |
412 | + |
413 | + out, err := sch.Coerce("foo", aPath) |
414 | + c.Assert(err, IsNil) |
415 | + c.Assert(out, Equals, "foo") |
416 | +} |
417 | + |
418 | +func (s *S) TestOneOf(c *C) { |
419 | + sch := schema.OneOf(schema.Const("foo"), schema.Const(42)) |
420 | + |
421 | + out, err := sch.Coerce("foo", aPath) |
422 | + c.Assert(err, IsNil) |
423 | + c.Assert(out, Equals, "foo") |
424 | + |
425 | + out, err = sch.Coerce(42, aPath) |
426 | + c.Assert(err, IsNil) |
427 | + c.Assert(out, Equals, 42) |
428 | + |
429 | + out, err = sch.Coerce("bar", aPath) |
430 | + c.Assert(out, IsNil) |
431 | + c.Assert(err, Matches, `<path>: unsupported value`) |
432 | +} |
433 | + |
434 | +func (s *S) TestBool(c *C) { |
435 | + sch := schema.Bool() |
436 | + |
437 | + out, err := sch.Coerce(true, aPath) |
438 | + c.Assert(err, IsNil) |
439 | + c.Assert(out, Equals, true) |
440 | + |
441 | + out, err = sch.Coerce(false, aPath) |
442 | + c.Assert(err, IsNil) |
443 | + c.Assert(out, Equals, false) |
444 | + |
445 | + out, err = sch.Coerce(1, aPath) |
446 | + c.Assert(out, IsNil) |
447 | + c.Assert(err, Matches, "<path>: expected bool, got 1") |
448 | +} |
449 | + |
450 | +func (s *S) TestInt(c *C) { |
451 | + sch := schema.Int() |
452 | + |
453 | + out, err := sch.Coerce(42, aPath) |
454 | + c.Assert(err, IsNil) |
455 | + c.Assert(out, Equals, int64(42)) |
456 | + |
457 | + out, err = sch.Coerce(int8(42), aPath) |
458 | + c.Assert(err, IsNil) |
459 | + c.Assert(out, Equals, int64(42)) |
460 | + |
461 | + out, err = sch.Coerce(true, aPath) |
462 | + c.Assert(out, IsNil) |
463 | + c.Assert(err, Matches, "<path>: expected int, got true") |
464 | +} |
465 | + |
466 | +func (s *S) TestFloat(c *C) { |
467 | + sch := schema.Float() |
468 | + |
469 | + out, err := sch.Coerce(float32(1.0), aPath) |
470 | + c.Assert(err, IsNil) |
471 | + c.Assert(out, Equals, float64(1.0)) |
472 | + |
473 | + out, err = sch.Coerce(float64(1.0), aPath) |
474 | + c.Assert(err, IsNil) |
475 | + c.Assert(out, Equals, float64(1.0)) |
476 | + |
477 | + out, err = sch.Coerce(true, aPath) |
478 | + c.Assert(out, IsNil) |
479 | + c.Assert(err, Matches, "<path>: expected float, got true") |
480 | +} |
481 | + |
482 | +func (s *S) TestString(c *C) { |
483 | + sch := schema.String() |
484 | + |
485 | + out, err := sch.Coerce("foo", aPath) |
486 | + c.Assert(err, IsNil) |
487 | + c.Assert(out, Equals, "foo") |
488 | + |
489 | + out, err = sch.Coerce(true, aPath) |
490 | + c.Assert(out, IsNil) |
491 | + c.Assert(err, Matches, "<path>: expected string, got true") |
492 | +} |
493 | + |
494 | +func (s *S) TestSimpleRegexp(c *C) { |
495 | + sch := schema.SimpleRegexp() |
496 | + out, err := sch.Coerce("[0-9]+", aPath) |
497 | + c.Assert(err, IsNil) |
498 | + c.Assert(out, Equals, "[0-9]+") |
499 | + |
500 | + out, err = sch.Coerce(1, aPath) |
501 | + c.Assert(out, IsNil) |
502 | + c.Assert(err, Matches, "<path>: expected regexp string, got 1") |
503 | + |
504 | + out, err = sch.Coerce("[", aPath) |
505 | + c.Assert(out, IsNil) |
506 | + c.Assert(err, Matches, `<path>: expected valid regexp, got "\["`) |
507 | +} |
508 | + |
509 | +func (s *S) TestList(c *C) { |
510 | + sch := schema.List(schema.Int()) |
511 | + out, err := sch.Coerce([]int8{1, 2}, aPath) |
512 | + c.Assert(err, IsNil) |
513 | + c.Assert(out, Equals, []interface{}{int64(1), int64(2)}) |
514 | + |
515 | + out, err = sch.Coerce(42, aPath) |
516 | + c.Assert(out, IsNil) |
517 | + c.Assert(err, Matches, "<path>: expected list, got 42") |
518 | + |
519 | + out, err = sch.Coerce([]interface{}{1, true}, aPath) |
520 | + c.Assert(out, IsNil) |
521 | + c.Assert(err, Matches, `<path>\[1\]: expected int, got true`) |
522 | +} |
523 | + |
524 | +func (s *S) TestMap(c *C) { |
525 | + sch := schema.Map(schema.String(), schema.Int()) |
526 | + out, err := sch.Coerce(map[string]interface{}{"a": 1, "b": int8(2)}, aPath) |
527 | + c.Assert(err, IsNil) |
528 | + c.Assert(out, Equals, map[interface{}]interface{}{"a": int64(1), "b": int64(2)}) |
529 | + |
530 | + out, err = sch.Coerce(42, aPath) |
531 | + c.Assert(out, IsNil) |
532 | + c.Assert(err, Matches, "<path>: expected map, got 42") |
533 | + |
534 | + out, err = sch.Coerce(map[int]int{1: 1}, aPath) |
535 | + c.Assert(out, IsNil) |
536 | + c.Assert(err, Matches, "<path>: expected string, got 1") |
537 | + |
538 | + out, err = sch.Coerce(map[string]bool{"a": true}, aPath) |
539 | + c.Assert(out, IsNil) |
540 | + c.Assert(err, Matches, `<path>\.a: expected int, got true`) |
541 | + |
542 | + // First path entry shouldn't have dots in an error message. |
543 | + out, err = sch.Coerce(map[string]bool{"a": true}, nil) |
544 | + c.Assert(out, IsNil) |
545 | + c.Assert(err, Matches, `a: expected int, got true`) |
546 | +} |
547 | + |
548 | +func (s *S) TestFieldMap(c *C) { |
549 | + fields := schema.Fields{ |
550 | + "a": schema.Const("A"), |
551 | + "b": schema.Const("B"), |
552 | + } |
553 | + sch := schema.FieldMap(fields, schema.Optional{"b"}) |
554 | + |
555 | + out, err := sch.Coerce(map[string]interface{}{"a": "A", "b": "B"}, aPath) |
556 | + c.Assert(err, IsNil) |
557 | + c.Assert(out, Equals, map[string]interface{}{"a": "A", "b": "B"}) |
558 | + |
559 | + out, err = sch.Coerce(42, aPath) |
560 | + c.Assert(out, IsNil) |
561 | + c.Assert(err, Matches, "<path>: expected map, got 42") |
562 | + |
563 | + out, err = sch.Coerce(map[string]interface{}{"a": "A", "b": "C"}, aPath) |
564 | + c.Assert(out, IsNil) |
565 | + c.Assert(err, Matches, `<path>\.b: expected "B", got "C"`) |
566 | + |
567 | + out, err = sch.Coerce(map[string]interface{}{"b": "B"}, aPath) |
568 | + c.Assert(out, IsNil) |
569 | + c.Assert(err, Matches, `<path>\.a: expected "A", got nothing`) |
570 | + |
571 | + // b is optional |
572 | + out, err = sch.Coerce(map[string]interface{}{"a": "A"}, aPath) |
573 | + c.Assert(err, IsNil) |
574 | + c.Assert(out, Equals, map[string]interface{}{"a": "A"}) |
575 | + |
576 | + // First path entry shouldn't have dots in an error message. |
577 | + out, err = sch.Coerce(map[string]bool{"a": true}, nil) |
578 | + c.Assert(out, IsNil) |
579 | + c.Assert(err, Matches, `a: expected "A", got true`) |
580 | +} |
581 | + |
582 | +func (s *S) TestSchemaMap(c *C) { |
583 | + fields1 := schema.FieldMap(schema.Fields{ |
584 | + "type": schema.Const(1), |
585 | + "a": schema.Const(2), |
586 | + }, nil) |
587 | + fields2 := schema.FieldMap(schema.Fields{ |
588 | + "type": schema.Const(3), |
589 | + "b": schema.Const(4), |
590 | + }, nil) |
591 | + sch := schema.FieldMapSet("type", []schema.Checker{fields1, fields2}) |
592 | + |
593 | + out, err := sch.Coerce(map[string]int{"type": 1, "a": 2}, aPath) |
594 | + c.Assert(err, IsNil) |
595 | + c.Assert(out, Equals, map[string]interface{}{"type": 1, "a": 2}) |
596 | + |
597 | + out, err = sch.Coerce(map[string]int{"type": 3, "b": 4}, aPath) |
598 | + c.Assert(err, IsNil) |
599 | + c.Assert(out, Equals, map[string]interface{}{"type": 3, "b": 4}) |
600 | + |
601 | + out, err = sch.Coerce(map[string]int{}, aPath) |
602 | + c.Assert(out, IsNil) |
603 | + c.Assert(err, Matches, `<path>\.type: expected supported selector, got nothing`) |
604 | + |
605 | + out, err = sch.Coerce(map[string]int{"type": 2}, aPath) |
606 | + c.Assert(out, IsNil) |
607 | + c.Assert(err, Matches, `<path>\.type: expected supported selector, got 2`) |
608 | + |
609 | + out, err = sch.Coerce(map[string]int{"type": 3, "b": 5}, aPath) |
610 | + c.Assert(out, IsNil) |
611 | + c.Assert(err, Matches, `<path>\.b: expected 4, got 5`) |
612 | + |
613 | + // First path entry shouldn't have dots in an error message. |
614 | + out, err = sch.Coerce(map[string]int{"a": 1}, nil) |
615 | + c.Assert(out, IsNil) |
616 | + c.Assert(err, Matches, `type: expected supported selector, got nothing`) |
617 | } |
It appears to do what it intends, but I do have a few questions...
[0]
+ path = append(path, "[", "?", "]") Coerce( rv.Index( i).Interface( ), path)
...
+ for i := 0; i != l; i++ {
+ path[len(path)-2] = strconv.Itoa(i)
+ elem, err := c.elem.
I wouldn't say this is *unclear*, but I did have to stop and think for a moment. I don't see any code path in which the "?" will actually be used, and I would favour a construction more like:
+ for i := 0; i != l; i++ { Coerce( rv.Index( i).Interface( ), itemPath)
+ itemPath = append(path, "[", strconv.Itoa(i). "]")
+ elem, err := c.elem.
Are there benefits to the original approach that I've missed?
[1]
+ vpath := append(path, ".", "?")
Same issue as [0].
[2]
Throughout: c, e, o, v, rv, l, vpath, newk, newv... while, again, these aren't actually unclear in context, they don't really reveal their intention very well (if at all). Are there forces that work against the use of longer variable names?
[3]
Finally, I'm concerned about the potential for drift -- both in Schema itself, and in individual schemas.
A language-agnostic schema format (where we define any given schema OAOO, and load it in both Go and Python) would solve the second problem. The first one is slightly trickier, but we could probably do it by encoding tests as sets of (schema-path, input, expected- error-or- serialized- output) ; we could then have one SchemaTest *loader* per language, which would replace the existing Schema unit tests -- and hopefully not need to change very much, if at all -- and should be sufficient to prevent drift between the implementations (a new Schema test is just a few files added to an existing directory; if a change is made in Python but not in Go, the next run of the Go tests will expose the issue). We'd still need tests for individual schemas, of course, but they could be done in the same format and share the benefits.
In practice, it may be that any single-language change would cause clear errors, quickly and reliably enough that we wouldn't get value from the extra scaffolding, but I'd need to be convinced of that ;).