add Getter methods for data structures

[willnorris]

as part of resolving #19, all data structures now use pointer field values. To ease the burden on developers to constantly perform nil checks, we should generate Get*() funcs on all our data structures, similar to what goprotobuf does. If a field is non-nil, return it. Otherwise, return the zero value for that type. So for example,

func (m *User) GetLogin() string {
    if m != nil && m.Login != nil {
        return *m.Login
    }
    return ""
}

Go provides good capabilities to read and manipulate go source code, so this can be completely generated. We might need to move all these types into a single data.go file, I'm not sure.

[willnorris]

Not yet starting work on this, but capturing some thoughts...

Go 1.4 added the go generate command which will make this a bit simpler (blog post: https://blog.golang.org/generate). The stringer command (https://golang.org/x/tools/cmd/stringer) should provide a nice starting point for writing a tool to generate accessor methods on our structs.

One question though is how to identify which structs should have generated accessors. go-github consists primarily of three types of structs:

• Services, used to break up the API surface into logical chunks
• GitHub resources
• Options, used to encapsulate optional arguments to various methods

We only want or need accessors on the GitHub resources. @bslatkin took the approach of using java-style comment annotations in bslatkin/joiner (blog post), which is kinda interesting, but noisier than I'd like:

// @joiner
type MyData struct{
    X float32
    Y float32
}

If we write the generator specific to go-github, we could do some really simple heuristics on type names. For example, skip anything that ends in "Service" or "Options". But an accessor generation tool like this does seem pretty generally useful, so I'd generally like the generator to be its own thing that maybe exposes a simple API that a go-github specific wrapper uses.

[parkr]

@willnorris Seems like a lot of work! I have been using Go's protobuf support recently to achieve this. Not only do you get the getters and setters for free, but you get a sweet new serialization format too! We have found it really nice at work. Maybe it's strange to define a .proto file for the primary purpose of generating setters and getters, but it'll do the job!

[willnorris]

@parkr Are you using protoc generated code in addition to hand-written structs, or instead of?

[willnorris]

Just to cut to the chase on why I ask... we tried to use protos for go-github but ran into lots of problems (see #19, notably #19 (comment) and #19 (comment)).

We could use protoc just to generate accessor methods but that creates two ongoing maintenance problems:

• github.proto would have to be kept in sync with the real go types
• the generated output from protoc would have to be edited to rip out all the stuff we don't want or need. That could be automated, but at that point you might as well generate it from the original go sources

Certainly, it's going to be some work to write the generation tool, but I don't think it will be too bad... stringer is 600 lines without tests. Plus, if designed well, it's something we'll only ever have to do once and can then be used by anyone else who similarly wants accessor methods for their pointer struct fields.

[willnorris]

though, it's also worth mentioning that I haven't looked at proto3 very closely and what its generated Go code looks like. There were some pretty big changes around the nullability of fields... basically, they take the Go approach and use the zero value for the type for unset fields. They handle nullability by having wrapper types that effectively do the same thing that we're doing by using pointers. I'm not sure what generated Go code looks like for those wrapper types, but I would imagine it's nothing too special, and would be even more annoying to work with than what we have today.

[parkr]

@parkr Are you using protoc generated code in addition to hand-written structs, or instead of?

The former. Your reasoning is sound – it sounds like you've been down this road. Just thought I'd throw that out there. 😄

[dmitshur]

@willnorris, what's the status of this issue (since you've self-assigned yourself to it)?

In #19 (comment), you said:

I'll open a new bug to look into generating convenience Get* methods similar to goprotobuf. In the meantime, users of the library will need to do their own nil checks.

But that was 2013, so I suspect this issue might be outdated and we probably don't actually want to do this anymore, do we?

[willnorris]

I never got to it, but I don't think it's outdated. Arguably, it's still something we should do.

[dmitshur]

I see that @gmlewis has created a PR #543 that resolves this issue. I wanted to discuss it here because I have some potential concerns.

go-github did not have those accessor methods since the issue was created in Aug 20, 2013, and since then there's been very little feedback (that's visible to me anyway) or demand for this feature. Put simply, very few people complained, and there are many users of this library. At the very least, this proves this feature is not critical to functionality.

With that said, I want to ask the question, does adding all those methods actually make this library better? Is it helpful?

My main concern right now is with the added API surface area and how this will affect godoc for go-github:

https://godoc.org/github.com/google/go-github/github

@gmlewis, have you looked at the before and after godoc, was it acceptably readable after this is implemented? I haven't seen what it looks like after yet. I think the godoc page is very readable and helpful right now, and I want to be careful not to compromise that.

Another question for @gmlewis, did you implement this feature because there's some demand (perhaps at Google) that I'm not aware of? Or was it simply because this issue was open and @willnorris said that we may still want to do it?

Finally, I'm curious how users of go-github feel about this. If this issue is resolved, would you want to use those new accessor methods? Or do you expect you'd simply access the fields directly (manually checking if they're nil, when neccessary; as we were forced to do up until now). Also, will this addition to the API help remove/simplify existing code that uses go-github?

I'm very open to discussion and definitely not against implementing this. I just wanted to ask the above questions first, in order to make/keep go-github library as good as possible. Thanks!

[gmlewis]

@gmlewis, have you looked at the before and after godoc, was it acceptably readable after this is implemented?

I was quite surprised to discover that godoc -http=:6060 does not show any of these new methods at all.
I thought possibly the hyphen in the filename (i.e. *-accessors.go) was messing it up so I tried underscores but got the same results. Surely I'm missing something.

Another question for @gmlewis, did you implement this feature because there's some demand (perhaps at Google) that I'm not aware of? Or was it simply because this issue was open and @willnorris said that we may still want to do it?

I implemented it for the following reasons:

• it sounded like a fun challenge and an ideal use of go generate, which I have been wanting to try out with a real-world use-case for quite some time;
• at Google, I'm working with protobufs every day (both proto2 and proto3 syntaxes) and miss the accessors provided for proto2 protobufs (where there is a very similar situation with everything-is-a-pointer);
• Will and I have been talking about how this would be nice-to-have for quite some time now;
• I just finished a large undertaking at work and wanted to blow off some steam and relax by solving a problem in Go.

Having said all that, I'm totally fine to leave the decision up to the community of go-github users.
If this is not perceived as providing benefit, I'm happy to close the PR... no harm, no foul... I had fun writing it. 😄

[parkr]

I would definitely find protobuf-style getters (generated from protobuf files?!) an excellent addition. If you were to also add .proto files, it would be easier to interop with large multi-language systems passing around this data for its work. Anyone who integrates with GitHub's platform could surely find that useful.

[dmitshur]

I was quite surprised to discover that godoc -http=:6060 does not show any of these new methods at all.

I tried looking at godoc -http=localhost:6060 and I don't see any issues. The new methods show up as expected. Are you sure you had the right import path and branch checked out?

Having looked at the godoc output after, I think the readability is still just fine. This package has a huge API either way, and is best used as a "reference" to look things up, not to so much to discover or get an overview. For that purpose, there's no harm to have extra methods on types.

The index is actually more complete-looking because you get a better idea of what fields are contained in types, instead of having to click on the type and see its definition in detail.

Index Before

Index After

When looking at a specific type, you have a harder time seeing the other types that follow (because there are now methods that follow it), but that doesn't seem to be a problem at all. You're typically looking at one thing at a time and using navigation/F shortcut to get to where you need to go.

Type Definition Before

Type Definition After

I implemented it for the following reasons

Thank you so much for explaining the reasons, it's very helpful for me to know those!

• it sounded like a fun challenge and an ideal use of go generate, which I have been wanting to try out with a real-world use-case for quite some time;
• I just finished a large undertaking at work and wanted to blow off some steam and relax by solving a problem in Go.

Absolutely, this is a fantastic fit for go generate, and the implementation looks quite nice from a quick glance. 👍

• at Google, I'm working with protobufs every day (both proto2 and proto3 syntaxes) and miss the accessors provided for proto2 protobufs (where there is a very similar situation with everything-is-a-pointer);
• Will and I have been talking about how this would be nice-to-have for quite some time now;

Makes sense.

I have used protobufs in the past, but decided to stop doing so. I don't miss the methods consciously, but I might decide to use them once they're around (I can't predict). I can imagine this would be useful for those who use/like protobuf accessors already.

I think my main potential concern is not a problem, so I don't have objections against doing this.

[willnorris]

If you were to also add .proto files, it would be easier to interop with large multi-language systems passing around this data for its work.

We are definitely not going to use proto for go-github itself for reasons discussed in #19. And just adding .proto files without actually using them seems weird and likely to get out of sync.

If I had it to do all over again, I'd much rather find a way to do this without the pointers, perhaps using proto3 as design inspiration. We do actually have an opportunity to rethink a lot of design decisions if we ever implement the new graphql API. That's something we should talk about at some point.

[arthurnn]

Hi @willnorris ,
If there is an interest on adding graphql API support here, I am interested in helping out on that. I will have some free time ahead on my schedule, and I am looking for a Go OS project to help out.
Is there anyone working on the graphql client here already?

cheers