Added Repository Collaborators API. #34
Conversation
| return false, nil, err | ||
| } | ||
| resp, err := s.client.Do(req, nil) | ||
| if resp.StatusCode == 204 { |
There was a problem hiding this comment.
use parseBoolResponse() here, since it includes some special handling of 404 responses.
There was a problem hiding this comment.
optionally, we could update parseBoolResponse() to explicitly look for 204 responses to indicate success. However, then I'm not sure what to do when receiving a non-204 response that also doesn't return an error (for example, any other 2** response code). Of course, that should never happen, but still worth coding against. As parseBoolResponse() is written today, a 200 response would still return true. I think that's probably okay.
There was a problem hiding this comment.
The only issue I can see here is the fact that 404 can be returned in 2 instances:
- The user is not a collaborator.
- The user does not exist.
The only way to tell these apart is through the error message Github returns. parseBoolResponse() will not return the error on a 404, so we would have no way of determining the reason of failure. Do we care about making a distinction between the two?
There was a problem hiding this comment.
hmm, interesting. I hadn't noticed that there actually are different error messages for those two different cases. Sadly, there's not a reliable machine readable way to distinguish between the two (parsing the error message feels too brittle). I'll look and see if this is true for the other methods as well, in which case we should probably update parseBoolResponse() to account for this.
There was a problem hiding this comment.
so none of the other methods I tested distinguish between a standard false response, and a false response because the requested resource doesn't exist. I think the fact that this method actually distinguishes between these is a fluke, so I'm not too worried about it. If we think anyone will care, I think I'd rather update parseBoolResponse() to suppress the 404 response if the error message is an exact match to the string "Not Found", and return it otherwise. But again, I don't think it's worth doing at this point. What do you think?
There was a problem hiding this comment.
I think that we shouldn't modify parseBoolResponse. Like you said, this seems to be a special edge case and not applicable anywhere else. The API docs also make no mention of this difference in 404's, so I think we're safe to treat them both as the same.
Plus if a user is not on GitHub then they are obviously not a collaborator on a project.
|
merged as b00b4e1 |
Update go-github version
Issue #33: Implementation for Repository Collaborators API.