search

dedis / protobuf

Reflection-based Protocol Buffers for Go
GNU General Public License v2.0
76 stars 15 forks source link

Support for golang map[] in fields #12

Closed liamsi closed 8 years ago

liamsi commented 8 years ago

As we switched from using gob to protobuf in the cothority project, we also need to encode/decode maps (protobuf supports maps since late 2014). Please review and merge when possible. I also merged a small fix (see #11) from @nikkolasg in this branch (other tests were failing without it).

bford commented 8 years ago

Looks fine to me!

nikkolasg commented 8 years ago

Good for me

ineiti commented 8 years ago

Nicolas just brought up that the protobuf-file creation is not working with maps. Ismail says it's not long. So let's do it...

liamsi commented 8 years ago

I just realised that I did not put the map support into generate.go (to generate .proto files). I'll have a look on that. If it's very easy I'll just add another commit, otherwise I'll open a new PR.

liamsi commented 8 years ago

Ah, you already commented on generate.go. Actually, I brought it up myself ;-) I tried now for longer half an hour, it's a little more complicated than I thought. If you want you can just go on and merge it. I'll just open I new PR on which I'll work tomorrow? Or if it can wait, I'll commit here tomorrow.

Update: As discussed with @ineiti it makes sense to add the changes here (do not merge until I'm done).

liamsi commented 8 years ago

I also added map support for the generation of .proto. Should be ready for merging now.

ineiti commented 8 years ago

Added some comment-nitpicks for the code.

liamsi commented 8 years ago

I think it's ready to merge. I incorporated all your comments (besides https://github.com/dedis/protobuf/pull/12#discussion_r49461274 and the comments on Nicolas code)

nikkolasg commented 8 years ago

Still Ok for me.