octokit / go-octokit

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

Working on "organization" APIs implementations and/or refactoring #97

Closed feiranchen closed 9 years ago

pengwynn commented 9 years ago

Good start, @lalalah. Let me know if you need help getting a test groove going. :zap:

feiranchen commented 9 years ago

Hey @pengwynn I have finished the test groove and added the plan struct. However the following API for editing cannot be implemented because Go does not support Patch and doesn't seem likely that it will in the near future.

https://developer.github.com/v3/orgs/#edit-an-organization

Here's a discussion in the Go Lang group talking about possibilities of implementing Patch.

https://groups.google.com/forum/#!topic/golang-nuts/GjngdEKsUXA

I can see how this API can be done in POST with some changes on the server side tho. More specifically, when handling a Post, we will apply the updated if and only if the organization already exists and that the user is eligible for making the update. I don't know how much change this would require on the server side.

Please let me know what you think. I will go ahead and work on something else before I hear back.

pengwynn commented 9 years ago

I think the mailing list topic was for the ParseForm method. NewRequest definitely accepts PATCH since we're using it in other places. Does that help?

feiranchen commented 9 years ago

Cool. My bad. Turns out Request is defined in every layer of the wrappers and I got caught up tracking them down! :stuck_out_tongue_closed_eyes:

The patch method as well as its test cases; testing both method and body of the patch http call.

NOTE: I think this is a typo: On the development page https://developer.github.com/v3/orgs/#response-3, I think the description field in the response should have be updated to:

"description": "GitHub, the company."

However the page currently says the following, which I think is wrong:

"description": "A great organization",

I have designed my test with my understanding. If the developer page is not a typo, please let me know and I'll revert it.

pengwynn commented 9 years ago

I think the description field in the response should have be updated

The sample responses in the docs are there to show the fields, not so much the values. I'd grab the fixtures you need for your tests via curl from the API. :grinning:

feiranchen commented 9 years ago

Yep I've been doing that! This is the first time I run into a situation where I don't have the permission to view the fields! Would you help me out?

How does this look otherwise?

feiranchen commented 9 years ago

For your convenience, the json fixture that I need for organzation(after update) but couldn't curl is here: https://github.com/octokit/go-octokit/blob/5e68239c4c25c486843cd3c4ae2b161b729ffdee/fixtures/organization_updated.json

feiranchen commented 9 years ago

updated per discussion in https://github.com/octokit/go-octokit/pull/98#discussion_r27810077

pengwynn commented 9 years ago

@feiranchen This is looking good, I like the recent changes. Anything else to implement here before we merge?

feiranchen commented 9 years ago

@pengwynn Thanks for your feed back! It's all good except the test fixture for this specific task was not curl-ed, but rather from the developer's documentation site. This is because I don't have the permission to curl some of these fields.

For your convenience, the json fixture that I need for organzation but couldn't curl is here: https://github.com/octokit/go-octokit/blob/5e68239c4c25c486843cd3c4ae2b161b729ffdee/fixtures/organization_updated.json

pengwynn commented 9 years ago

@feiranchen No problem. I just updated the fixture.

pengwynn commented 9 years ago

@feiranchen I think we'll have to update the test after changing the fixture. If you need me to take another look, just add the needs-review label back on there.

feiranchen commented 9 years ago

Synced with test cases. No high level update.

pengwynn commented 9 years ago

:cake: