Skip to content
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

Updates to master #55

Closed
wants to merge 8 commits into from
Closed

Updates to master #55

wants to merge 8 commits into from

Conversation

renatosnrg
Copy link
Contributor

Hi Josh,

I've added the site parameter to consumer oauth options. It's related to the issue #51, when we try to authorize from a previous stored user token (authorize_from_access).

Without the site parameter the consumer HTTP object couldn't be created correctly. It works with "get_request_token" because the request_token_url is a complete url, this way the HTTP object is created directly. But using "authorize_from_access" the oauth gem needs the site option to create the correct HTTP object.

Renato

@marckohlbrugge
Copy link

Just wanted to +1 this pull request. Renato's fork made the gem work for me.

@jeremyw
Copy link

jeremyw commented Jul 31, 2011

+1 This works for me too. If there's a reason this can't be merged, please let us know so we can clean it up.

@boyvanamstel
Copy link

+1, this also fixed the gem for me.

@pedrodelgallego
Copy link

+1

1 similar comment
@comp615
Copy link

comp615 commented Aug 21, 2011

+1

@ciberch
Copy link

ciberch commented Aug 26, 2011

+1 merge it please

@lstone
Copy link

lstone commented Aug 27, 2011

+1 For merge please!

@berkes
Copy link

berkes commented Aug 30, 2011

Doesn't this pull-request do a lot more then just fix issue #51? AFAIK (but I am no github-guru) the pull-request should only contain SHA: 90b6641, not?

@tadast
Copy link
Contributor

tadast commented Aug 30, 2011

@berkes, it is possible to cherrypick commits, but not with github web UI yet AFAIK.
+1 because @renatosnrg did a good job and tested it with recent linkedin API result 'cassetes'.

edit: although changes should also be reflected in examples/documentation I guess.

@ciberch
Copy link

ciberch commented Aug 31, 2011

@berkes agreed I am using not only the fix but also the companies API. Should @renatosnrg split it up ? Not sure if this is what is holding the committers/Josh from accepting it.

@berkes
Copy link

berkes commented Aug 31, 2011

I am very willing to make this into separate pull-requests, if that is what is keeping @josh or @wynnetherland from committing.

@lstone
Copy link

lstone commented Aug 31, 2011

I'm gonna give this a +2 now, please :)

@renatosnrg
Copy link
Contributor Author

I believe @josh can pick the commits he wants to merge, he doesn't need to merge all of them. By the way, Github automatically added my other commits to this pull request. Originally, only the first one was used to generate this pull request.

@ryanatwork
Copy link
Collaborator

Hey all...

I merged in the pull request for the site paramater and bumped the version of the gem

@ryanatwork ryanatwork closed this Sep 4, 2011
@lstone
Copy link

lstone commented Sep 4, 2011

Thank you!

@ciberch
Copy link

ciberch commented Sep 6, 2011

@renatosnrg can you please submit another pull request for the companies piece which was also in this pull request ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet