-
Notifications
You must be signed in to change notification settings - Fork 951
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
Rename id param in get_object() to object_id #198
base: master
Are you sure you want to change the base?
Conversation
In order to specify 'id' as a keyword argument, we cannot use 'id' as a positional argument name.
In the issue you opened, you mentioned |
The method is:
So as is it definitely takes in keyword arguments. I assumed this was so that you could pass in keyword arguments to finally get them to go through In the line I posted that fails: I'm in a situation where all I have is a URL, and I'm trying to find the comments associated with it. |
I think you want to do Let me know if I am missing something, or if there is another object/node in the API that uses "id" as a parameter. |
Actually, my FB Graph query is slightly incorrect, but the same issue still holds. Technically the breaking line would be:
Note: this is for v2.1+ |
The comments are not traditional comments, they're via this: https://developers.facebook.com/docs/plugins/comments.
But even with that specific use case aside, you can still query the graph like so:
which would translate to:
and is currently broken. |
Does Would Regardless, I think moving away from using Python built-ins as variable names is a good idea. |
The documentation is surprisingly poor, but yes, everything I've found to date has shown that If you had the |
Any chance you would be willing to write a unit test for this? |
Sure! |
Done. |
class TestGetObject(FacebookTestCase): | ||
def test_get_object(self): | ||
# The value we are passing as 'fields' is valid only in 2.1+. | ||
graph = facebook.GraphAPI(access_token=facebook.get_app_access_token( |
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.
facebook.get_app_access_token
is deprecated and might be removed in the near future. Can we use the GraphAPI().get_app_access_token
here?
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.
Done. My bad on that, I followed the same pattern as a test above it which I guess hasn't been updated yet.
Updated. |
+1 When is 2.0.0 planned to be released? |
In order to specify 'id' as a keyword argument,
we cannot use 'id' as a positional argument name.
This is a fix for: #197