dedis / protobuf

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

No serializable fields #43

Closed aounleonardo closed 6 years ago

aounleonardo commented 6 years ago

I went with the simple solution of checking the length of en.Bytes() before returning it in Encode(). And that is because of the equivalence: "len(en.Bytes()) == 0" iff "struct has no serializable fields"

The alternative I can see us doing is: in (en *encoder) message, if no field.CanSet() was true in the loop, we can do something. However, similarly to the issue #30 no other method than Encode can return errors.

Closes #38

ineiti commented 6 years ago

Does that hold? What about the following structure:

struct Empty{
  Empty *string
}

func main(){
  buf, err := protobuf.Encode(&Empty{})
}

That would trigger your test, no?

kc1212 commented 6 years ago

I'm wondering if doing a panic will stop us from serialising legitimate empty structs. For example, in https://github.com/dedis/cothority/blob/master/status/service/struct.go our request is empty. I think this PR will break the status service. Having a struct with only private fields might also be legitimate. Should we consider printing a warning instead of panic?

aounleonardo commented 6 years ago

as @kc1212 suggested, I pushed a change that just prints a warning instead of panicking, should we keep it like that or panic?

ineiti commented 6 years ago

@kc1212 - what do you think? Now it looks good to me.

kc1212 commented 6 years ago

Yes, it looks good @ineiti