Uh oh!
There was an error while loading. Please reload this page.
Adds support for GET /repos/:owner/:repo/collaborators/:username/permission - #534
Adds support for GET /repos/:owner/:repo/collaborators/:username/permission#534alindeman wants to merge 2 commits into
Conversation
dmitshur
left a comment
There was a problem hiding this comment.
This is looking good. A few questions.
| // member has for a given repository. | ||
| type RepositoryPermissionLevel struct { | ||
| // Possible values: "admin", "write", "read", "none" | ||
| Permission string `json:"permission"` |
There was a problem hiding this comment.
What's the reason you decided to use a string here instead of *string, as is commonly done in other structs?
| // Possible values: "admin", "write", "read", "none" | ||
| Permission string `json:"permission"` | ||
| User *User `json:"user"` |
There was a problem hiding this comment.
Was there a reason to omit ,omitempty option for both User and Permission, unlike all other structs we have?
alindeman
commented
Jan 30, 2017
@shurcooL 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. Of course, that's assuming new API routes will not be added in the future that only return subsets of attributes. If you think it'd be better if they matched up with the typical convention of pointers and |
Thanks for answering. That makes sense. I wanted to discuss this a bit, and share some thoughts/findings. My comment ended up being lengthy, so I decided to take it out from this PR and make it a separate issue, so that others can weigh in their opinions there, without derailing this PR. Please see #537 (comment). |
4a7dd75 to
f39108fComparealindeman
commented
Jan 31, 2017
gmlewis
left a comment
There was a problem hiding this comment.
Thank you, @alindeman and @shurcooL!
If you want to address a small change, you may, or we could leave it for #536. Either way is fine with me.
| if err != nil { | ||
| return nil, resp, err | ||
| } | ||
| return rpl, resp, err |
There was a problem hiding this comment.
We are about to explicitly return nil when there is no error, so maybe you could start this practice:return rpl, resp, nil
See #536.
dmitshur
left a comment
There was a problem hiding this comment.
@shurcooL I've amended my commit based on the conclusion in #537
All right, sounds good.
LGTM from my side (but see @gmlewis's comment). Thanks @alindeman!
@gmlewis Sounds good. I thought the convention was a bit odd, but I admit to blindly following it for consistency. Fixed in the latest push. |
gmlewis
commented
Jan 31, 2017
Thank you, @alindeman! |
…ission Closesgoogle#534. Change-Id: I851f803f931697aa1684ee4323d984bd80ffbbda
…ission Closesgoogle#534. Change-Id: I851f803f931697aa1684ee4323d984bd80ffbbda
GitHub API docs: https://developer.github.com/v3/repos/collaborators/#review-a-users-permission-level