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

Replace urllib with Python Requests #68

Closed
wants to merge 6 commits into from
Closed

Replace urllib with Python Requests #68

wants to merge 6 commits into from

Conversation

mvid
Copy link

@mvid mvid commented Jan 29, 2013

looking for some feedback, seeing if it is possible to get this merged mainline?

get_app_access_token rewritten with requests

get_access_token_from_code rewritten with requests

request rewritten with requests

all graphapi calls converted to use requests

deprecating old commands
@martey
Copy link
Member

martey commented Jan 29, 2013

Some of these commits (mvid@a5521cc for example) are non-starters, but I will review the others when I have time.

@mvid
Copy link
Author

mvid commented Jan 29, 2013

Understandable. I would prefer to merge this mainline than to fork it. I will roll those all back.

@martey
Copy link
Member

martey commented Feb 6, 2013

Why were get_access_token_from_code and get_app_access_token moved?

@mvid
Copy link
Author

mvid commented Feb 6, 2013

They were not removed, but replaced with Graph object implementations. The functions still exist, but simply call the a Graph instance function. They are also marked deprecated.

The functions were documented as being external to the Graph object because they returned query-args instead of json. Now that the Graph objects can handle query-arg responses, they don't need to be separate.

@martey
Copy link
Member

martey commented Feb 6, 2013

I think it makes sense to keep them undeprecated because of the way the GraphAPI object is currently initialized. Which these changes, one must create a GraphAPI object and then run either get_access_token_from_code or get_app_access_token to in order to get an access token.

I could see newcomers being confused about the fact that the returned access token is not automatically applied to the GraphAPI object. If we change it, I could see people upgrading being burned by this new behavior.

I think it might make sense to change the __init__ of the GraphAPI object to fix this, but this pull request is definitely not the place for do this, since switching to Requests is a big enough change as is.

@mvid
Copy link
Author

mvid commented Feb 6, 2013

Makes sense. I will remove the deprecation warnings.

@skarra
Copy link

skarra commented Feb 13, 2013

Marty - having trouble with google appengine. Are your changes supposed to work on GAE?

@martey
Copy link
Member

martey commented Feb 13, 2013

@skarra, are you having trouble with the changes in this pull request?

If not, you should either use the Google group (if you need technical support) or file a new issue (if you think you have found a bug).

@mvid
Copy link
Author

mvid commented Feb 13, 2013

@skarra yes they are supposed to work. Feel free to email or twitter message me your issues, if they are related to this pull request.

@mvid mvid mentioned this pull request May 8, 2013
@mvid
Copy link
Author

mvid commented May 8, 2013

I have addressed the deprecation issue in my PR. Apologies for the amount of time that has passed.

Closing this PR, as the new one is the active one.

@mvid mvid closed this May 8, 2013
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.

3 participants