Add support for ,inline flag with struct values.

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)

[niemeyer, 2013-06-18 03:47:03]

Please take a look.

[jameinel, 2013-06-19 04:47:34]

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

[niemeyer, 2013-06-19 14:12:56]

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?

[rog, 2013-06-19 15:11:12]

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.

[niemeyer, 2013-06-19 17:53:16]

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.

[niemeyer, 2013-06-19 18:06:45]

*** 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