pyjuju

Merge lp:~niemeyer/pyjuju/go-rename-short-types into lp:pyjuju/go

  1. go-rename-short-types
  2. Merge into go
Proposed by Gustavo Niemeyer
Status: Merged
Approved by: Gustavo Niemeyer
Approved revision: 8
Merge reported by: Gustavo Niemeyer
Merged at revision: not available
Proposed branch: lp:~niemeyer/pyjuju/go-rename-short-types
Merge into: lp:pyjuju/go
Prerequisite: lp:~niemeyer/pyjuju/go-initial-formula-meta
Diff against target: 224 lines (+36/-36)
4 files modified
formula/formula.go (+3/-3)
formula/formula_test.go (+16/-16)
schema/schema.go (+11/-11)
schema/schema_test.go (+6/-6)
To merge this branch: bzr merge lp:~niemeyer/pyjuju/go-rename-short-types
Reviewer Review Type Date Requested Status
Kapil Thangavelu (community) Approve
William Reade (community) Approve
Review via email: mp+73242@code.launchpad.net

Description of the change

The Go implementation has schema.M/L shorthand types that, as Kapil correctly points out, are not readable. This change fixes it.

To post a comment you must log in.
Revision history for this message
Jamu Kakar (jkakar) wrote :

This is a nice change, thanks. I find the short variable and type
names that I've seen in idiomatic Go code make the code hard to read.

Revision history for this message
William Reade (fwereade) wrote :

Definitely more readable, +1

review: Approve
Revision history for this message
Gustavo Niemeyer (niemeyer) wrote :

Hey Jamu. Good to have you around.

We were just debating in the call recently that it may indeed be detrimental to
readability in some cases, and Kapil properly pointed out that this was the
case with this work. In other cases, I do feel like he short variable names
actually help.. we went overboard with long names in Ensemble (much more than
in Landscape), and I'd like to attempt to step back a bit now with that work.
Go also helps that, because it gives some additional context to the short
names, and prevents silly mistakes.

Please feel free to drop in with actual review comments whenever you please.
It's very welcome.

Revision history for this message
Kapil Thangavelu (hazmat) wrote :

looks good, thanks

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'formula/formula.go'
2--- formula/formula.go 2011-08-29 14:58:01 +0000
3+++ formula/formula.go 2011-08-29 14:58:01 +0000
4@@ -66,7 +66,7 @@
5 if err != nil {
6 return
7 }
8- m := v.(schema.M)
9+ m := v.(schema.MapType)
10 meta = &Meta{}
11 meta.Name = m["name"].(string)
12 // Schema decodes as int64, but the int range should be good
13@@ -113,7 +113,7 @@
14 func (c ifaceExpC) Coerce(v interface{}, path []string) (newv interface{}, err os.Error) {
15 s, err := stringC.Coerce(v, path)
16 if err == nil {
17- newv = schema.M{
18+ newv = schema.MapType{
19 "interface": s,
20 "limit": c.limit,
21 "optional": false,
22@@ -129,7 +129,7 @@
23 if err != nil {
24 return
25 }
26- m := v.(schema.M)
27+ m := v.(schema.MapType)
28 if _, ok := m["limit"]; !ok {
29 m["limit"] = c.limit
30 }
31
32=== modified file 'formula/formula_test.go'
33--- formula/formula_test.go 2011-08-29 14:58:01 +0000
34+++ formula/formula_test.go 2011-08-29 14:58:01 +0000
35@@ -107,36 +107,36 @@
36 // Shorthand is properly rewritten
37 v, err := e.Coerce("http", path)
38 c.Assert(err, IsNil)
39- c.Assert(v, Equals, schema.M{"interface": "http", "limit": nil, "optional": false})
40+ c.Assert(v, Equals, schema.MapType{"interface": "http", "limit": nil, "optional": false})
41
42 // Defaults are properly applied
43- v, err = e.Coerce(schema.M{"interface": "http"}, path)
44- c.Assert(err, IsNil)
45- c.Assert(v, Equals, schema.M{"interface": "http", "limit": nil, "optional": false})
46-
47- v, err = e.Coerce(schema.M{"interface": "http", "limit": 2}, path)
48- c.Assert(err, IsNil)
49- c.Assert(v, Equals, schema.M{"interface": "http", "limit": int64(2), "optional": false})
50-
51- v, err = e.Coerce(schema.M{"interface": "http", "optional": true}, path)
52- c.Assert(err, IsNil)
53- c.Assert(v, Equals, schema.M{"interface": "http", "limit": nil, "optional": true})
54+ v, err = e.Coerce(schema.MapType{"interface": "http"}, path)
55+ c.Assert(err, IsNil)
56+ c.Assert(v, Equals, schema.MapType{"interface": "http", "limit": nil, "optional": false})
57+
58+ v, err = e.Coerce(schema.MapType{"interface": "http", "limit": 2}, path)
59+ c.Assert(err, IsNil)
60+ c.Assert(v, Equals, schema.MapType{"interface": "http", "limit": int64(2), "optional": false})
61+
62+ v, err = e.Coerce(schema.MapType{"interface": "http", "optional": true}, path)
63+ c.Assert(err, IsNil)
64+ c.Assert(v, Equals, schema.MapType{"interface": "http", "limit": nil, "optional": true})
65
66 // Invalid data raises an error.
67 v, err = e.Coerce(42, path)
68 c.Assert(err, Matches, "<path>: expected map, got 42")
69
70- v, err = e.Coerce(schema.M{"interface": "http", "optional": nil}, path)
71+ v, err = e.Coerce(schema.MapType{"interface": "http", "optional": nil}, path)
72 c.Assert(err, Matches, "<path>.optional: expected bool, got nothing")
73
74- v, err = e.Coerce(schema.M{"interface": "http", "limit": "none, really"}, path)
75+ v, err = e.Coerce(schema.MapType{"interface": "http", "limit": "none, really"}, path)
76 c.Assert(err, Matches, "<path>.limit: unsupported value")
77
78 // Can change default limit
79 e = formula.IfaceExpander(1)
80- v, err = e.Coerce(schema.M{"interface": "http"}, path)
81+ v, err = e.Coerce(schema.MapType{"interface": "http"}, path)
82 c.Assert(err, IsNil)
83- c.Assert(v, Equals, schema.M{"interface": "http", "limit": int64(1), "optional": false})
84+ c.Assert(v, Equals, schema.MapType{"interface": "http", "limit": int64(1), "optional": false})
85 }
86
87
88
89=== modified file 'schema/schema.go'
90--- schema/schema.go 2011-08-29 14:58:01 +0000
91+++ schema/schema.go 2011-08-29 14:58:01 +0000
92@@ -9,11 +9,11 @@
93 "strings"
94 )
95
96-// All map types used in the schema package are of type M.
97-type M map[interface{}]interface{}
98+// All map types used in the schema package are of type MapType.
99+type MapType map[interface{}]interface{}
100
101-// All the slice types generated in the schema package are of type L.
102-type L []interface{}
103+// All the slice types generated in the schema package are of type ListType.
104+type ListType []interface{}
105
106 // The Coerce method of the Checker interface is called recursively when
107 // v is being validated. If err is nil, newv is used as the new value
108@@ -200,7 +200,7 @@
109 // provided slice value fails to be processed, processing will stop
110 // and return with the obtained error.
111 //
112-// The coerced output value has type schema.L.
113+// The coerced output value has type schema.ListType.
114 func List(elem Checker) Checker {
115 return listC{elem}
116 }
117@@ -218,7 +218,7 @@
118 path = append(path, "[", "?", "]")
119
120 l := rv.Len()
121- out := make(L, 0, l)
122+ out := make(ListType, 0, l)
123 for i := 0; i != l; i++ {
124 path[len(path)-2] = strconv.Itoa(i)
125 elem, err := c.elem.Coerce(rv.Index(i).Interface(), path)
126@@ -235,7 +235,7 @@
127 // value fails to be coerced, processing stops and returns with the
128 // underlying error.
129 //
130-// The coerced output value has type schema.M.
131+// The coerced output value has type schema.MapType.
132 func Map(key Checker, value Checker) Checker {
133 return mapC{key, value}
134 }
135@@ -254,7 +254,7 @@
136 vpath := append(path, ".", "?")
137
138 l := rv.Len()
139- out := make(M, l)
140+ out := make(MapType, l)
141 keys := rv.MapKeys()
142 for i := 0; i != l; i++ {
143 k := keys[i]
144@@ -281,7 +281,7 @@
145 // individually. If a field fails to be processed, processing stops
146 // and returns with the underlying error.
147 //
148-// The coerced output value has type schema.M.
149+// The coerced output value has type schema.MapType.
150 func FieldMap(fields Fields, optional Optional) Checker {
151 return fieldMapC{fields, optional}
152 }
153@@ -309,7 +309,7 @@
154 vpath := append(path, ".", "?")
155
156 l := rv.Len()
157- out := make(M, l)
158+ out := make(MapType, l)
159 for k, checker := range c.fields {
160 vpath[len(vpath)-1] = k
161 var value interface{}
162@@ -334,7 +334,7 @@
163 // field processes the map correctly. If no checker processes
164 // the selector value correctly, an error is returned.
165 //
166-// The coerced output value has type schema.M.
167+// The coerced output value has type schema.MapType.
168 func FieldMapSet(selector string, maps []Checker) Checker {
169 fmaps := make([]fieldMapC, len(maps))
170 for i, m := range maps {
171
172=== modified file 'schema/schema_test.go'
173--- schema/schema_test.go 2011-08-29 14:58:01 +0000
174+++ schema/schema_test.go 2011-08-29 14:58:01 +0000
175@@ -166,7 +166,7 @@
176 sch := schema.List(schema.Int())
177 out, err := sch.Coerce([]int8{1, 2}, aPath)
178 c.Assert(err, IsNil)
179- c.Assert(out, Equals, schema.L{int64(1), int64(2)})
180+ c.Assert(out, Equals, schema.ListType{int64(1), int64(2)})
181
182 out, err = sch.Coerce(42, aPath)
183 c.Assert(out, IsNil)
184@@ -185,7 +185,7 @@
185 sch := schema.Map(schema.String(), schema.Int())
186 out, err := sch.Coerce(map[string]interface{}{"a": 1, "b": int8(2)}, aPath)
187 c.Assert(err, IsNil)
188- c.Assert(out, Equals, schema.M{"a": int64(1), "b": int64(2)})
189+ c.Assert(out, Equals, schema.MapType{"a": int64(1), "b": int64(2)})
190
191 out, err = sch.Coerce(42, aPath)
192 c.Assert(out, IsNil)
193@@ -218,7 +218,7 @@
194
195 out, err := sch.Coerce(map[string]interface{}{"a": "A", "b": "B"}, aPath)
196 c.Assert(err, IsNil)
197- c.Assert(out, Equals, schema.M{"a": "A", "b": "B"})
198+ c.Assert(out, Equals, schema.MapType{"a": "A", "b": "B"})
199
200 out, err = sch.Coerce(42, aPath)
201 c.Assert(out, IsNil)
202@@ -239,7 +239,7 @@
203 // b is optional
204 out, err = sch.Coerce(map[string]interface{}{"a": "A"}, aPath)
205 c.Assert(err, IsNil)
206- c.Assert(out, Equals, schema.M{"a": "A"})
207+ c.Assert(out, Equals, schema.MapType{"a": "A"})
208
209 // First path entry shouldn't have dots in an error message.
210 out, err = sch.Coerce(map[string]bool{"a": true}, nil)
211@@ -260,11 +260,11 @@
212
213 out, err := sch.Coerce(map[string]int{"type": 1, "a": 2}, aPath)
214 c.Assert(err, IsNil)
215- c.Assert(out, Equals, schema.M{"type": 1, "a": 2})
216+ c.Assert(out, Equals, schema.MapType{"type": 1, "a": 2})
217
218 out, err = sch.Coerce(map[string]int{"type": 3, "b": 4}, aPath)
219 c.Assert(err, IsNil)
220- c.Assert(out, Equals, schema.M{"type": 3, "b": 4})
221+ c.Assert(out, Equals, schema.MapType{"type": 3, "b": 4})
222
223 out, err = sch.Coerce(map[string]int{}, aPath)
224 c.Assert(out, IsNil)

Subscribers

People subscribed via source and target branches