Policy on using pointers/values in structs for receiving data from GitHub servers.

[dmitshur]

I'm making this issue to discuss a point that came up while reviewing #534. /cc @alindeman

Background

In #534, @alindeman answered my questions:

@shurcooL: What's the reason to use string value rather than *string pointer, as is commonly done in other structs?
@shurcooL: What's the reason to omit ,omitempty option, contrary to most similar structs we have?

@alindeman: Thinking back to the conversation we had over in #512, I thought that since--as far as I can tell--these attributes will always be returned from the API, we could specify them as non-pointer types.

One key difference is that the conversation in #512 was primarily about the request structs - structs that are used to send values to github servers. The affect on structs for receiving and unmarshaling data from github servers was inadvertent and discovered at the end, and we decided to go ahead with it because it was very minor. This PR is all about the structs for receiving from the start, that's why I want to think about it more carefully.

As I understood it up until now (before doing the following research), the main reason pointers are used for most receiving structs is to be able to tell when a field is absent vs when it's present but set to zero.

That's what I saw in #79. That links to #19 where the initial discussion took place, and 3072d06 is the implementation commit. However, as I read over #19 in more detail now, I was surprised to see that the main motivation to have pointers there seemed to be for the ability to set fields, because at the time many structs were reused for setting and getting. /cc @willnorris

It's interesting that we've seemingly evolved towards dedicated Request structs for editing/updating operations (e.g., https://godoc.org/github.com/google/go-github/github#IssueRequest and https://godoc.org/github.com/google/go-github/github#Issue; https://godoc.org/github.com/google/go-github/github#BranchRestrictionsRequest and https://godoc.org/github.com/google/go-github/github#BranchRestrictions, also see #528, etc.). This was typically forced on us by certain new GitHub endpoints that had differences in the JSON they expect when editing/updating a resource. Especially whey they have required fields in the request.

In #512 we discussed and came to the conclusion that it makes sense to use values rather than pointers for required fields in the request structs.

Conclusion

So in the end, I think that it's valid to also use values rather than pointers for mandatory fields (i.e., those that are guaranteed to be present by GitHub API).

However, one limitation of that is that GitHub API documentation doesn't typically specify fields in the response as mandatory/required, so it's hard to be sure they'll always be present.

One concern is that it also introduces some inconsistency against all the other structs we have that use pointers everywhere.

Next Steps

As a result, I'm a little torn and unsure about what we should do. Do we maintain consistency and keep using pointers everywhere? Do we allow values in certain places where it seems like they're always going to be included? Do we try to convert more things to values?

We're also more constrained in changing existing structs because of desire to not break API without meaningful gains, whereas we can choose what to do for new structs.

[dmitshur]

Having done the research on history of pointers, and writing up the above issue, I am starting to feel that the path of least resistance is to continue to always use pointers for receiving structs.

The other paths seem to be more expensive and not really worth it from what I can imagine. But I'm open to discussion. I'm curious what others think.

Unless there are convincing arguments otherwise, in a few days, I will likely make the recommendation in #534 to use pointers for the receiving structs in order to maintain consistency with the large set of existing structs and not deviating.

[alindeman]

Unless there are convincing arguments otherwise, in a few days, I will likely make the recommendation in #534 to use pointers for the receiving structs in order to maintain consistency with the large set of existing structs and not deviating.

Solid analysis. I agree with this conclusion.

[dmitshur]

I think this question is resolved, we've come to a conclusion. Closing since there's nothing actionable left to do.

We can continue to use this issue as reference on the topic, or to post followup comments. (Although it's gonna be harder to find...)

[willnorris]

Sorry I missed this until now, but yeah I completely agree :) As I've said in a few other places, if I had it to do over again, I'm not sure that I would have done the same (it's worth noting that proto3 has moved away from nillable fields). If and when we ever do a rewrite (as will likely be required for the graphql API), then that is a good time to reconsider the design choice.

[weppos]

As I've said in a few other places, if I had it to do over again, I'm not sure that I would have done the same

Sorry to jump into the conversation, but this is a very interesting feedback. I'm having pretty much the same issue for a client to our API (which has to interact to a service in a very similar way to GitHub API, and for which I took some inspiration long time ago from this client).

I'd be interested to get a sense on what path you would follow could you start today.

I was aware of this decision for quite a while, and I followed with interest the evolution of the client, way before for e.g. you provided the accessors). I've been using this go client lately, and to be fair it's not such a big effort from a consumer POV, due to the helpers and the accessors you provide.

However, from a maintainer POV, I've a very mixed feeling about this change. Especially because of the amount of changes and boilerplate to generate (thinking of all the accessors).

I like consistency, hence the idea of defining a type with some fields string and others *string really bothers me.

The two solutions I am currently evaluating are:

1. switch to pointers
2. use Request objects (e.g. DomainRequest vs Domain)

Given in most cases the Request object is a subset of the actual object (even in github, there are fields such as created_at/updated_at that you don't set, or other association fields), I am leaning towards that idea.

But I'm curious, generally speaking, what would be the approach you would follow today.

[dmitshur]

I like using Request objects (e.g., IssueComment vs IssueCommentEdit). It leverages the Go type system to ensure you don't try to set fields that shouldn't be (e.g., see a recent issue #885).

However, that is somewhat orthogonal to whether you use pointers in the domain types. You still need to use pointers if you want to be able to tell whether the field was included and set to zero value, or not included. Although maybe that's rarely useful.

But, this applies to REST APIs only. I am starting to rely more on GraphQL APIs these days, which use a completely different approach.

[weppos]

However, that is somewhat orthogonal to whether you use pointers in the domain types. You still need to use pointers if you want to be able to tell whether the field was included and set to zero value, or not included. Although maybe that's rarely useful.

Indeed. The main difference is that you won't have to update the whole codebase, nor force all types to have pointers.

Thanks for the feedback.