Merge lp:~niemeyer/goyaml/inline-struct into lp:goyaml

[Gustavo Niemeyer]

Add support for ,inline flag with struct values.

This also adds the code to do inlining of maps, as supported by bson
(the code was copied from mgo/bson), but it's disabled for the moment
as I'll need more time to implement it.

https://codereview.appspot.com/10366044/

[Gustavo Niemeyer (niemeyer)]

Reviewers: mp+169981_code.launchpad.net,

Message:
Please take a look.

Description:
Add support for ,inline flag with struct values.

This also adds the code to do inlining of maps, as supported by bson
(the code was copied from mgo/bson), but it's disabled for the moment
as I'll need more time to implement it.

https://code.launchpad.net/~niemeyer/goyaml/inline-struct/+merge/169981

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/10366044/

Affected files:
   A [revision details]
   M decode.go
   M decode_test.go
   M encode.go
   M encode_test.go
   M goyaml.go

[John A Meinel (jameinel)]

The encode side of code looks reasonable, but what happens on decode if
you have ',inline' set ?

https://codereview.appspot.com/10366044/

[Gustavo Niemeyer (niemeyer)]

On 2013/06/19 04:47:34, jameinel wrote:
> The encode side of code looks reasonable, but what happens on decode
if you have
> ',inline' set ?

I don't understand the question. The logic is symmetrical, and tested?

https://codereview.appspot.com/10366044/

[Roger Peppe (rogpeppe)]

LGTM with some more tests and some other thoughts.

https://codereview.appspot.com/10366044/diff/1/encode.go
File encode.go (right):

https://codereview.appspot.com/10366044/diff/1/encode.go#newcode122
encode.go:122: value = in.FieldByIndex(info.Inline)
cool, i didn't know you could use FieldByIndex on arbitrary
non-embedded structs.

is there a particular reason you can't unify Num and Inline, BTW?

https://codereview.appspot.com/10366044/diff/1/encode_test.go
File encode_test.go (right):

https://codereview.appspot.com/10366044/diff/1/encode_test.go#newcode211
encode_test.go:211: },
i'd like to see more tests of the inline functionality.
in particular:
- more than one inline struct
- nested inlines.
- errors from field conflicts.

https://codereview.appspot.com/10366044/diff/1/goyaml.go
File goyaml.go (right):

https://codereview.appspot.com/10366044/diff/1/goyaml.go#newcode124
goyaml.go:124: // struct.
// Any clash of field names will result in an error.
?

https://codereview.appspot.com/10366044/diff/1/goyaml.go#newcode155
goyaml.go:155: InlineMap int
i'd find some comments on these fields helpful for understanding the
code.

https://codereview.appspot.com/10366044/diff/1/goyaml.go#newcode163
goyaml.go:163: Inline []int
// Inline holds the set of field indexes for
// looking up the given field.

https://codereview.appspot.com/10366044/diff/1/goyaml.go#newcode232
goyaml.go:232: case reflect.Struct:
what about pointers to struct?

it's reasonable, i think, to be able to do:

type Foo struct {
    One string
    Other *Other `goyaml:",inline"`
}

so you can inline the same kinds of things that you can
embed.

also, for the future, you might want to consider allowing
interface types too - the treatment could be similar to
inline maps.

https://codereview.appspot.com/10366044/

[Gustavo Niemeyer (niemeyer)]

Thanks for the review, Roger.

https://codereview.appspot.com/10366044/diff/1/encode.go
File encode.go (right):

https://codereview.appspot.com/10366044/diff/1/encode.go#newcode122
encode.go:122: value = in.FieldByIndex(info.Inline)
On 2013/06/19 15:11:12, rog wrote:
> is there a particular reason you can't unify Num and Inline, BTW?

No big reason, other than Num being very cheap, both to implement and to
execute, and being the common case by far.

https://codereview.appspot.com/10366044/diff/1/encode_test.go
File encode_test.go (right):

https://codereview.appspot.com/10366044/diff/1/encode_test.go#newcode211
encode_test.go:211: },
On 2013/06/19 15:11:12, rog wrote:
> i'd like to see more tests of the inline functionality.
> in particular:
> - more than one inline struct
> - nested inlines.
> - errors from field conflicts.

Done.

https://codereview.appspot.com/10366044/diff/1/goyaml.go
File goyaml.go (right):

https://codereview.appspot.com/10366044/diff/1/goyaml.go#newcode124
goyaml.go:124: // struct.
On 2013/06/19 15:11:12, rog wrote:
> // Any clash of field names will result in an error.
> ?

That's not specific to ,inline. I've added a note both to Marshal and
Unmarshal pointing out that conflicting names result in a runtime error.

https://codereview.appspot.com/10366044/diff/1/goyaml.go#newcode155
goyaml.go:155: InlineMap int
On 2013/06/19 15:11:12, rog wrote:
> i'd find some comments on these fields helpful for understanding the
code.

Improved the struct names and documentation.

https://codereview.appspot.com/10366044/diff/1/goyaml.go#newcode163
goyaml.go:163: Inline []int
On 2013/06/19 15:11:12, rog wrote:
> // Inline holds the set of field indexes for
> // looking up the given field.

Not quite, but documented appropriately.

https://codereview.appspot.com/10366044/diff/1/goyaml.go#newcode232
goyaml.go:232: case reflect.Struct:
On 2013/06/19 15:11:12, rog wrote:
> what about pointers to struct?

We can add support for them later, but I'd prefer to keep it simple at
this time. Pointers makes the implementation more complex because the
struct has to be initialized if one of the fields is found, and it also
means we're creating garbage on every iteration of a set of documents.
We can do it, but I'd prefer to wait until people need it. I've never
had anyone asking for it with bson.

https://codereview.appspot.com/10366044/

[lp:~niemeyer/goyaml/inline-struct]

46. By Gustavo Niemeyer on 2013-06-19

Review by Roger plus overall naming improvements.

47. By Gustavo Niemeyer on 2013-06-19

Merged from tip.

48. By Gustavo Niemeyer on 2013-06-19

go fmt

[Gustavo Niemeyer (niemeyer)]

*** Submitted:

Add support for ,inline flag with struct values.

This also adds the code to do inlining of maps, as supported by bson
(the code was copied from mgo/bson), but it's disabled for the moment
as I'll need more time to implement it.

R=jameinel, rog
CC=
https://codereview.appspot.com/10366044

https://codereview.appspot.com/10366044/

=== modified file 'decode.go'
--- decode.go	2013-04-30 00:42:36 +0000
+++ decode.go	2013-06-18 03:47:27 +0000
@@ -445,7 +445,13 @@
 			continue
 		}
 		if info, ok := fieldsMap[name.String()]; ok {
-			d.unmarshal(n.children[i+1], out.Field(info.Num))
+			var field reflect.Value
+			if info.Inline == nil {
+				field = out.Field(info.Num)
+			} else {
+				field = out.FieldByIndex(info.Inline)
+			}
+			d.unmarshal(n.children[i+1], field)
 		}
 	}
 	return true

=== modified file 'decode_test.go'
--- decode_test.go	2013-05-29 17:23:20 +0000
+++ decode_test.go	2013-06-18 03:47:27 +0000
@@ -317,6 +317,15 @@
 			B int "-"
 		}{1, 0},
 	},
+
+	// Struct inlining
+	{
+		"a: 1\nb: 2\n",
+		&struct {
+			A int
+			C struct{ B int } `yaml:",inline"`
+		}{1, struct{ B int }{2}},
+	},
 }
 
 func (s *S) TestUnmarshal(c *C) {

=== modified file 'encode.go'
--- encode.go	2013-04-28 04:28:40 +0000
+++ encode.go	2013-06-18 03:47:27 +0000
@@ -115,7 +115,12 @@
 	}
 	e.mappingv(tag, func() {
 		for _, info := range fields.List {
-			value := in.Field(info.Num)
+			var value reflect.Value
+			if info.Inline == nil {
+				value = in.Field(info.Num)
+			} else {
+				value = in.FieldByIndex(info.Inline)
+			}
 			if info.OmitEmpty && isZero(value) {
 				continue
 			}

=== modified file 'encode_test.go'
--- encode_test.go	2013-04-29 15:13:04 +0000
+++ encode_test.go	2013-06-18 03:47:27 +0000
@@ -200,6 +200,15 @@
 		}{1, 2},
 		"a: 1\n",
 	},
+
+	// Struct inlining
+	{
+		&struct {
+			A int
+			C struct{ B int } `yaml:",inline"`
+		}{1, struct{ B int }{2}},
+		"a: 1\nb: 2\n",
+	},
 }
 
 func (s *S) TestMarshal(c *C) {

=== modified file 'goyaml.go'
--- goyaml.go	2013-04-17 18:57:10 +0000
+++ goyaml.go	2013-06-18 03:47:27 +0000
@@ -119,6 +119,10 @@
 //     flow         Marshal using a flow style (useful for structs,
 //                  sequences and maps.
 //
+//     inline       Inline the struct it's applied to, so its fields
+//                  are processed as if they were part of the outer
+//                  struct.
+//
 // In addition, if the key is "-", the field is ignored.
 //
 // For example:
@@ -131,7 +135,7 @@
 //     goyaml.Marshal(&T{F: 1}} // Returns "a: 1\nb: 0\n"
 //
 func Marshal(in interface{}) (out []byte, err error) {
-	//defer handleErr(&err)
+	defer handleErr(&err)
 	e := newEncoder()
 	defer e.destroy()
 	e.marshal("", reflect.ValueOf(in))
@@ -146,8 +150,9 @@
 // The code in this section was copied from gobson.
 
 type structFields struct {
-	Map  map[string]fieldInfo
-	List []fieldInfo
+	Map       map[string]fieldInfo
+	List      []fieldInfo
+	InlineMap int
 }
 
 type fieldInfo struct {
@@ -155,6 +160,7 @@
 	Num       int
 	OmitEmpty bool
 	Flow      bool
+	Inline    []int
 }
 
 var fieldMap = make(map[reflect.Type]*structFields)
@@ -176,7 +182,8 @@
 
 	n := st.NumField()
 	fieldsMap := make(map[string]fieldInfo)
-	fieldsList := make([]fieldInfo, n)
+	fieldsList := make([]fieldInfo, 0, n)
+	inlineMap := -1
 	for i := 0; i != n; i++ {
 		field := st.Field(i)
 		if field.PkgPath != "" {
@@ -193,24 +200,7 @@
 			continue
 		}
 
-		// XXX Drop this after a few releases.
-		if s := strings.Index(tag, "/"); s >= 0 {
-			recommend := tag[:s]
-			for _, c := range tag[s+1:] {
-				switch c {
-				case 'c':
-					recommend += ",omitempty"
-				case 'f':
-					recommend += ",flow"
-				default:
-					msg := fmt.Sprintf("Unsupported flag %q in tag %q of type %s", string([]byte{uint8(c)}), tag, st)
-					panic(externalPanic(msg))
-				}
-			}
-			msg := fmt.Sprintf("Replace tag %q in field %s of type %s by %q", tag, field.Name, st, recommend)
-			panic(externalPanic(msg))
-		}
-
+		inline := false
 		fields := strings.Split(tag, ",")
 		if len(fields) > 1 {
 			for _, flag := range fields[1:] {
@@ -219,6 +209,8 @@
 					info.OmitEmpty = true
 				case "flow":
 					info.Flow = true
+				case "inline":
+					inline = true
 				default:
 					msg := fmt.Sprintf("Unsupported flag %q in tag %q of type %s", flag, tag, st)
 					panic(externalPanic(msg))
@@ -227,6 +219,41 @@
 			tag = fields[0]
 		}
 
+		if inline {
+			switch field.Type.Kind() {
+			//case reflect.Map:
+			//	if inlineMap >= 0 {
+			//		return nil, errors.New("Multiple ,inline maps in struct " + st.String())
+			//	}
+			//	if field.Type.Key() != reflect.TypeOf("") {
+			//		return nil, errors.New("Option ,inline needs a map with string keys in struct " + st.String())
+			//	}
+			//	inlineMap = info.Num
+			case reflect.Struct:
+				sfields, err := getStructFields(field.Type)
+				if err != nil {
+					return nil, err
+				}
+				for _, finfo := range sfields.List {
+					if _, found := fieldsMap[finfo.Key]; found {
+						msg := "Duplicated key '" + finfo.Key + "' in struct " + st.String()
+						return nil, errors.New(msg)
+					}
+					if finfo.Inline == nil {
+						finfo.Inline = []int{i, finfo.Num}
+					} else {
+						finfo.Inline = append([]int{i}, finfo.Inline...)
+					}
+					fieldsMap[finfo.Key] = finfo
+					fieldsList = append(fieldsList, finfo)
+				}
+			default:
+				//panic("Option ,inline needs a struct value or map field")
+				panic("Option ,inline needs a struct value field")
+			}
+			continue
+		}
+
 		if tag != "" {
 			info.Key = tag
 		} else {
@@ -238,11 +265,11 @@
 			return nil, errors.New(msg)
 		}
 
-		fieldsList[len(fieldsMap)] = info
+		fieldsList = append(fieldsList, info)
 		fieldsMap[info.Key] = info
 	}
 
-	fields = &structFields{fieldsMap, fieldsList[:len(fieldsMap)]}
+	fields = &structFields{fieldsMap, fieldsList, inlineMap}
 
 	fieldMapMutex.Lock()
 	fieldMap[st] = fields