-
Notifications
You must be signed in to change notification settings - Fork 31
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
AngelList API role updates. #17
Conversation
This seems good to my angellist-api-untrained eyes. 👍 |
@@ -25,6 +30,7 @@ module StartupRoles | |||
# @example Get users involved in startup with ID 1234, and their roles. | |||
# AngellistApi.get_startup_roles(:startup_id => 1234) | |||
def get_startup_roles(options={}) | |||
options.merge!(:v => 1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're not exactly following semver very well with this gem because a.) somehow, someone released a 1.0 version to Rubygems without it being tagged or updated in code (found when I tried to push 9a3ea88 to rubygems), and b.) AngelList considers the API beta and isn't promising any API stability yet. So we really should still be on 0.x releases (I'm tempted to revert to a 0.x scheme and yank the 1.x versions honestly).
Anyhow, it seems pretty nasty to force this parameter on users without a major version bump, especially because there's no new method or name change and, as written, library users can't even choose to override this to keep the old behavior while they work to update their code. Seems like we ought to just document that the user must pass the v
option himself to get the new behavior and that it will become default in the future (possibly emit a warning if it's called without the option, to warn users of AngelList's deprecation).
So the |
# @example Get | ||
# AngellistApi.startup_roles(1234) | ||
def startup_roles(id, options={}) | ||
get("1/startups/#{id}/roles") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, we forgot to pass the options to get
here, so it's impossible to pass the direction 😖
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh! Want me to fix that?
3 role methods added:
Integration tests included (with saved vcr cassete).