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

Expand the documentation for get_connection #400

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Drauda1
Copy link

@Drauda1 Drauda1 commented Nov 28, 2017

The field expansion feature of GraphAPI can be efficiently used through the 'fields' parameter of the get_connections method. This means that the user can obtain info from both the referenced edge in 'connection_name', and the node that is connected to 'object_id' through said edge. Doing so is very useful, as it can reduce the amount of calls to the API, and help the user access richer data through a simpler query. This information is not intuitively acquired from either the Facebook SDK for Python docs, or the GraphAPI reference guide.

The field expansion feature of GraphAPI can be efficiently used through the 'fields' parameter of the get_connections method. This means that the user can obtain info from both the referenced edge in 'connection_name', and the node that is connected to 'object_id' through said edge. Doing so is very useful, as it can reduce the amount of calls to the API, and help the user access richer data through a simpler query. This information is not intuitively acquired from either the Facebook SDK for Python docs, or the GraphAPI reference guide.
Copy link
Member

@martey martey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that this pull request is useful and that it makes the documentation clearer. The only changes I would request would be a hyperlink to the specific part of the Graph API documentation that explains field expansion, and fixing the seven errors that doc8 found:

docs/api.rst:174: D002 Trailing whitespace
docs/api.rst:175: D002 Trailing whitespace
docs/api.rst:176: D002 Trailing whitespace
docs/api.rst:177: D002 Trailing whitespace
docs/api.rst:189: D002 Trailing whitespace
docs/api.rst:174: D001 Line too long
docs/api.rst:178: D001 Line too long

docs/api.rst Outdated
or more fields of the edge specified in connection_name, or one or more
fields/edges of the node connected to the given object_id by said edge.
This parameter allows for nested queries to the GraphAPI, supported by the
field expansion feature. See the GraphAPI docs for syntax and further examples.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think a link to the specific section of the Graph API documentation would be useful here.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Marty, I completely forgot about this simple request you had. Out of the blue my GitHub account popped in my mind and I have requested a pull with the changes you asked for. Thank you for this super useful SDK!

Copy link
Member

@martey martey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you rebase your commits so that they use the latest master commit as a base? rebase -p master should do the trick.

This should allow the automated tests to run successfully.

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.

2 participants