Go-github: Support named structs in gen-accessors.go.

[dmitshur]

@gmlewis, I wanted to ask you a question regarding what you said in https://github.com/google/go-github/pull/577#discussion_r104304110:

(Note that named structs themselves don't get accessors [intentionally]... there is no GetUser method, for example, because you can just do something like resp.User.GetName() for example and it will just work... even if resp.User == nil... does that answer the question?)

Did you take into account that the above won't work if accessing more than 1 field in a single expression?

For example, if the caller wants to get the name of user that created an issue, this will be okay as you mentioned:

name := issue.User.GetName()

Because of the if u == nil { return "" } check in (u *User) GetName().

But this will panic:

planName := issue.User.Plan.GetName()

// panic: runtime error: invalid memory address or nil pointer dereference

Because issue.User is (*github.User)(nil), and accessing Plan field of that causes a run-time panic:

The following rules apply to selectors:
[...]
5. If x is of pointer type and has the value nil and x.f denotes a struct field, assigning to or evaluating x.f causes a run-time panic.

(Source: https://golang.org/ref/spec#Selectors.)

Did you take that into account when making the decision to omit the named structs? If so, what's the expected work around? If not, do we want to make a change, or is it fine?

This is just a question at this point, I don't have any suggestions for next steps yet.

[gmlewis]

I thought about that at the time but didn't pursue it, thinking we would rarely run into the issue.

This is an excellent point, however, and I'm thinking that we should indeed support it so that we can have:

planName := issue.GetUser().GetPlan().GetName()

agreed?

If so, I'm happy to make the changes, or feel free to take it or file an issue and let anyone who wants to take it.

[dmitshur]

Ok, I see.

Since this isn't time sensitive, I'd like to take a few days to think about it before coming to a decision.

[gmlewis]

PR #653 demonstrates a nice example case for why this might be useful.

@shurcooL - point of order... can we convert this "question" issue into a "feature request" issue, or do we need to open a new issue and point to the question issue (this one)?

[dmitshur]

Feel absolutely free to convert it to an actionable issue. I see no reason not to.

[gmlewis]

I will not label this issue as "good for beginner" but it would be a great learning experience for whomever wishes to take it on. :smile:

I'll let this issue sit for a while in case someone wishes to contribute, then take it on myself if there are no other takers.

[sahildua2305]

@gmlewis I'd like to take it on. Though, I'm not really sure what needs to be done. I see the use case that this feature will enable. Will appreciate some pointers to get me started! :)

[gmlewis]

Ah yes, this item is definitely non-trivial. "Not good for beginner" might be an understatement.
Feel free to punt if you find yourself getting overwhelmed... no harm, no foul.

To start, let's take a look at type User struct around line 20 of users.go.
Most of its members are simple types like *string, *int, *bool, etc.
All of these simple fields should be found with an accessor in github-accessors.go. (I see them starting around line 6943).

But you'll notice that there is no func (u *User) GetPlan() *Plan found in github-accessors.go.
One could argue that I was being lazy. Or one might argue that I thought there wasn't a need for them. Either way, as we have discussed in this issue, it might be super-handy to have.

Your job, should you choose to accept it, is to add accessors for all the struct types... not directly by editing github-accessors.go, mind you, but by editing gen-accessors.go such that they are auto-generated each time.

The "catch" is that these accessors return nil if they are passed a nil receiver so that they can easily be chained as in the example above. Never will you get a panic if you use the package's accessors.

Once again, this is non-trivial. If you still want to do this, thank you very much... your help is greatly appreciated! If you decide that this is not for you, let me know, and I'm happy to unassign you with no harm done.

Thank you!

[sahildua2305]

Interesting! I like the challenge. Thanks for the detailed description of the task. I'd like to go through the gen-accessors.go first and try to understand how it works as of now.

[sahildua2305]

Okay, now I understand how gen-accessors.go works. I see that the named structs are going to be of *ast.Ident type. I'm still trying to decide on what the GetPlan accessor (for example) is going to look like in github-accessors.go.

Is it going to look like this? Or is there some catch in this?

func (u *User) GetPlan() *Plan {
    if u == nil || u.Plan == nil {
        return nil
    }
    return *u.Plan
}

EDIT: Now I see the catch with nil return. 🤔. I think what I wrote above should work. What do you think?

[gmlewis]

That looks great except the return line should be return u.Plan to match the return type.

[sahildua2305]

Oh yes, should be return u.Plan. Thanks for pointing out!

Cool, I'll start working on it then.

[gmlewis]

Additionally, you will want to add appropriate comments to make the resulting godoc look good. :smile:
Thank you, @sahildua2305!

[gmlewis]

Oh, and keep them sorted. :smile:

[sahildua2305]

Additionally, you will want to add appropriate comments to make the resulting godoc look good.

Here, are you referring to the comment that goes above every accessor method?

[gmlewis]

Yes, exactly.