Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add documentation on the autogenerated methods #198

Open
ericlagergren opened this issue Sep 15, 2016 · 6 comments
Open

Add documentation on the autogenerated methods #198

ericlagergren opened this issue Sep 15, 2016 · 6 comments

Comments

@ericlagergren
Copy link

ericlagergren commented Sep 15, 2016

I'm using custom types and have run into some issues.

For example, I've a UUID type (type UUID [16]byte). I added the Unmarshal method which calls the already-existing UnmarshalBinary method. However, UnmarshalBinary checks to make sure the slice passed to it is 16 bytes long.

In the generated tests a buffer with a length of 0 is passed, causing errors.

It'd be nice if there was some documentation so people like me who want to abuse protobufs a bit have some sort of guarantees about how the methods work.

(Aside: it'd be lovely if UnmarshalBinary could work in lieu of Unmarshal, similarly with UnmarshalJSON and UnmarshalText :-)

Also, thanks for this library. It's terrific.

@ericlagergren
Copy link
Author

ericlagergren commented Sep 15, 2016

I was thinking something like:

// Unmarshaler unmarshals a binary (protobuf?) representation of itself.
// It will always pass t.Size() bytes as its argument
// etc.
type Unmarshaler interface {
    Unmarshal([]byte) error
}

@awalterschulze
Copy link
Member

Minimal comments on public APIs are welcome. Maybe you would like to make a pull request?

I think a generated slice of zero bytes will only be passed if this is what your NewPopulated function returned? But maybe I am wrong?

Checking whether something implements the Unmarshaler or UnmarshalBinary takes time and slows down unmarshaling.

Don't get me wrong cusomtype definitely has its issues. Its the second hardest feature to maintain and its not even designed correctly. If only proto3 had added a 128bit type with time.Time and time.Duration then no one would even use customtype in my opinion. Most people just want [16]byte for IPv6 and Uuid.

@ericlagergren
Copy link
Author

ericlagergren commented Sep 15, 2016

I could try making a PR. Thing is, I'm not quite sure what documentation to write (thus this issue).

Ahhh I wish proto3 had a 128 bit type. Would make life so much easier :)

Here's are the methods: https://gist.github.com/EricLagergren/f798984c161dbdb6428372ee5d990963

I receive a whole bunch of:

authpbpb_test.go:1194: seed = 1473976066036462923, err = line 6: uuid: UUID must be exactly 16 bytes long, got 0 bytes uuid.UUID: <

@ericlagergren
Copy link
Author

Actually, this is causing it:

string: id:<} } 12:/* unknown wire type 6 */ 6:97 6:/* unknown wire type 7 */ } 12:862151469 12 { 5:926376550 5:909128802 5:1698182709 12:/* unknown wire type 6 */ } } 12:/* unexpected EOF */ 12:/* unexpected EOF */ 6:/* unexpected EOF */ } >name:"YbxAOnPbfBeaqioJ3gPoqlmleCQ9T3KRDMES4rWWNFzCxsZsfYuyJYZqF3CaNmBvDZfFjQzTur" domain:"ogLc3CubeX79n23Vj6Mrkxvf0gZyp4AvLyjKHo5nFafDQtfRg1DqilX3iCw" template:"703EQDwD4jK5uF1Msw5b8u" 

tok.unquoted has a length of 0.

@ericlagergren
Copy link
Author

Fouuuuuund the issue. I'll create a new one for posterity.

@awalterschulze
Copy link
Member

Like you say what documentation to write can be quite tricky. Someone would have to go through the code generator and look at all the options and try to make a concise sentence or two to document the generated method.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants