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

github-accessors.go: Missing accessors for non-Struct type fields #775

Closed
sahildua2305 opened this Issue Nov 8, 2017 · 5 comments

Comments

Projects
None yet
3 participants
@sahildua2305
Collaborator

sahildua2305 commented Nov 8, 2017

While working on #770, I noticed that we are creating accessors only for fields which have a struct type. I looked further into it today and confirmed the same. Is this intentional? If so, why?
/cc @gmlewis @shurcooL

@gmlewis

This comment has been minimized.

Show comment
Hide comment
@gmlewis

gmlewis Nov 8, 2017

Collaborator

I'm not sure I understand what you mean.
https://github.com/google/go-github/blob/master/github/github-accessors.go#L24
returns a string, for example.

Collaborator

gmlewis commented Nov 8, 2017

I'm not sure I understand what you mean.
https://github.com/google/go-github/blob/master/github/github-accessors.go#L24
returns a string, for example.

@dmitshur

This comment has been minimized.

Show comment
Hide comment
@dmitshur

dmitshur Nov 9, 2017

Member

@sahildua2305 Can you give some examples of fields that are missing accessors?

Member

dmitshur commented Nov 9, 2017

@sahildua2305 Can you give some examples of fields that are missing accessors?

@sahildua2305

This comment has been minimized.

Show comment
Hide comment
@sahildua2305

sahildua2305 Nov 11, 2017

Collaborator

For example -

CurrentUserOrganizationURLs []string `json:"current_user_organization_urls,omitempty"`

here - CurrentUserOrganizationURLs won't have any accessor because of the following logic -
se, ok := field.Type.(*ast.StarExpr)

Collaborator

sahildua2305 commented Nov 11, 2017

For example -

CurrentUserOrganizationURLs []string `json:"current_user_organization_urls,omitempty"`

here - CurrentUserOrganizationURLs won't have any accessor because of the following logic -
se, ok := field.Type.(*ast.StarExpr)

@dmitshur

This comment has been minimized.

Show comment
Hide comment
@dmitshur

dmitshur Nov 11, 2017

Member

The type of that particular field CurrentUserOrganizationURLs is []string. It’s not a pointer.

Not having an accessor for it seems reasonable, since the accessors exist for pointer fields to help prevent nil pointer dereference panics when a field is missing. Since there’s no pointer here, there’s nothing to protect against. The following code can never panic:

fmt.Println(len(feeds.CurrentUserOrganizationURLs))

Does that make sense? Or am I missing something? What would having an accessor for that field accomplish?

Member

dmitshur commented Nov 11, 2017

The type of that particular field CurrentUserOrganizationURLs is []string. It’s not a pointer.

Not having an accessor for it seems reasonable, since the accessors exist for pointer fields to help prevent nil pointer dereference panics when a field is missing. Since there’s no pointer here, there’s nothing to protect against. The following code can never panic:

fmt.Println(len(feeds.CurrentUserOrganizationURLs))

Does that make sense? Or am I missing something? What would having an accessor for that field accomplish?

@sahildua2305

This comment has been minimized.

Show comment
Hide comment
@sahildua2305

sahildua2305 Nov 11, 2017

Collaborator

Oh right! Makes sense. I knew I was missing something. Thanks for clearing it out. 💯

Collaborator

sahildua2305 commented Nov 11, 2017

Oh right! Makes sense. I knew I was missing something. Thanks for clearing it out. 💯

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