Bug: Conflict with Size fieldname and Size method name about protobuf HOT 12 CLOSED

I have made this fix on the sizeunderscore branch.
Here is the commit
8d709bd
If someone could please check it out before I merge it into master that would be great.

from protobuf.

merged

from protobuf.

Is there something special about the Size method? I mean, is it the part of some stdlib interface, or it's a gogoprotobuf addition?
I'm asking because it seams weird to change the name of Size field for struct in whole codebase to use it with this great library :)

from protobuf.

It might be, but not yet.
There is/was a TODO in the golang/protobuf to check for a Size method.
To change the method name now would cause a backwards incompatible change if anyone was calling the Size method.
And this is possible, since Size is a public method.

You can use gogoproto.customname to change the field name again.

from protobuf.

I'm wondering, is it possible to change Size method to SizePb or something like that?
It can be backward-compatible at least for the lib itself. We can just try both interfaces:

type sizer interface{
  SizePb() int
}
func (this *codec) Marshal(v interface{}) ([]byte, error) {
  var n int
  if psz, ok := v.(sizer); ok {
    n = psz.SizePb()
  } else if sz, ok := v.(interface{ Size() int }); ok { // compatibility
    n = sz.Size()
  }
  // ...
}

For lib users we can have one more option to generate Size methods instead of SizePb. It will still work for both variants since we check each interface on Marshal.

from protobuf.

That is a lot of effort for very little pay off.
Why is it so important?

from protobuf.

This is the TODO I was talking about
https://github.com/gogo/protobuf/blob/master/proto/encode.go#L277

from protobuf.

It's important because Size is a pretty common name for both methods and fields. String method has its interface already defined in Go, so it's ok to rename all user's fields and methods to avoid collisions. But Size is different. There is already interface with a Size method in stdlib with a completely different meaning: Size() int64 in os.FileInfo. So I just can't satisfy it and convert my struct to PB at the same time. I think there are many other examples of the same issue (dropbox@4ceca3a was done for a reason).

The issue can be easily resolved with fork, as Dropbox guys done, but it would be better to have it in official repo, I believe :)

from protobuf.

You make a compelling argument, but this is still a breaking change.
You are also not the first to request this, but I don't know how many people's code I will be breaking if I change this.
So the only way would be to add another extension called protosizer or something that generates a ProtoSize() method and then having Marshal check for both methods.
Pretty much like you suggested.

from protobuf.

I agree, another extension is a much better way than just renaming everything at once.
I will fill a PR some time later, if you don't mind.

from protobuf.

Ok that will be great.

extensions name: protosizer
method name: ProtoSize

Maybe the marshal plugin could check which of protosizer or sizer is used and then call that method.
If none is used and the user provides its own Size or ProtoSize method. Marshal should type assert something like you have proposed above. What do you think?

from protobuf.

I should look at the code more closely, but it sounds right to check for used extension. I'll post a PR in a few days then. I hope I will get enough time to test it properly :)

from protobuf.