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

Refactor with Python Requests #76

Closed
wants to merge 2 commits into from
Closed

Refactor with Python Requests #76

wants to merge 2 commits into from

Conversation

mvid
Copy link

@mvid mvid commented May 8, 2013

My last PR [#68] became garbled because of some github project owner changes. Opening this one.

@martey martey mentioned this pull request Jun 1, 2013
@mvid
Copy link
Author

mvid commented Jun 4, 2013

I would love to get this merged, and I currently have plenty of availability to make changes if they are required. @martey what are your thoughts on this?

@martey
Copy link
Member

martey commented Jun 4, 2013

Hi @mvid,

Would it be possible to create a new pull request with only the changes to add Requests? This would mean not including commits like mvid/facebook-sdk@0dc3d51 or mvid/facebook-sdk@c301c00, as well as changes that introduce new unrelated functionality like mvid/facebook-sdk@fb93ff0.

@mvid
Copy link
Author

mvid commented Jun 4, 2013

Of course. Rebasing and will push changed soon. @jbombien what would you like to do with your changes?

mvid and others added 2 commits June 4, 2013 12:14
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
@mvid
Copy link
Author

mvid commented Jun 4, 2013

@martey the changeset is rebased not to include those spurious commits.

@jbombien I had to remove your graph search addition. After this pull request goes through, you should create a new request against master.

@jbombien
Copy link

jbombien commented Jun 5, 2013

Hey,
it's okay for me. I'll make a pull request for the graphsearch function later.

@mvid
Copy link
Author

mvid commented Jun 15, 2013

Any word on this @martey ?

@mvid
Copy link
Author

mvid commented Aug 27, 2013

Bump? @martey

@martey
Copy link
Member

martey commented Aug 28, 2013

I have not merged this request because we do not have a test suite to automate testing, and I do not currently have the time to manually test these changes.

The quickest way to get this merged would be for someone to submit pull requests containing tests. I am on my honeymoon right now (with intermittent Internet access), so it is difficult for me to do this myself until next month at the earliest.

@mvid mvid closed this Sep 18, 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.

None yet

3 participants