search

octokit / go-octokit

Simple Go wrapper for the GitHub API
https://github.com/octokit/go-octokit
MIT License
258 stars 79 forks source link

Gists endpoint refactoring and expansion #106

Closed dannysperling closed 9 years ago

dannysperling commented 9 years ago

Refactoring and fixing the gists endpoint to use the current style. Gists have been refactored, but still working on expanding to have full coverage of the endpoint.

pengwynn commented 9 years ago

Looking good.

dannysperling commented 9 years ago

Endpoint should be finished - looks like it's not building because of the issue with comparing strings to Hyperlinks. Is there an easy way to fix that problem?

pengwynn commented 9 years ago

@dannysperling It looks like testify changed behavior. :frowning:

dannysperling commented 9 years ago

@pengwynn I remembered there was one thing I was confused by. In the "CheckStar" method, the API returns a 404 "Not Found" if there is no star on the gist:

https://developer.github.com/v3/gists/#check-if-a-gist-is-starred

That counts as an error in the result, since a 404 is in fact an error message. Right now, I have the test expecting an error in that case, but this seems unclean. Is there a better way to handle this?

Side note: why would any API return a 404 in an expected circumstance? That seems like bad design...

pengwynn commented 9 years ago

That counts as an error in the result, since a 404 is in fact an error message. Right now, I have the test expecting an error in that case, but this seems unclean. Is there a better way to handle this?

@dannysperling I think that's just fine. :+1:

Side note: why would any API return a 404 in an expected circumstance? That seems like bad design...

This is actually a pretty common RESTful approach. Though Not Found is in the client error range, it's not a bad thing. You're just asking the server for a resource and it's saying "I don't have that". The client can then easily wrap up that logic as a Boolean existence check.

feiranchen commented 9 years ago

@dannysperling @pengwynn Bringing this back to your attention because I noticed that some of the test fixtures are not curl-ed from the API but manually constructed. I've been advised to do the former before. If this was intended feel free to ignore this message.