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

Add since option to Page#posts and Page#videos #92

Closed
wants to merge 3 commits into from

Conversation

kangkyu
Copy link
Contributor

@kangkyu kangkyu commented Jul 27, 2017

No description provided.

@coveralls
Copy link

Coverage Status

Coverage decreased (-14.08%) to 85.918% when pulling b58566d on kangkyu:add-since into edbf464 on Fullscreen:master.

def videos(options = {})
path_query = "#{id}/videos?fields=id,title,description,created_time,length,comments.limit(0).summary(true),likes.limit(0).summary(true),reactions.limit(0).summary(true)"
path_query << "&since=#{options[:since]}" if options[:since]
videos = Funky::Connection::API.fetch_all(path_query)

Choose a reason for hiding this comment

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

@kangkyu The is_array: true part is missing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I used fetch_all instead of fetch method for token-based pagination. I thought time-based pagination (fetch with is_array: true) can be hard to work with since or until parameter

@@ -46,8 +46,10 @@ def videos
#
# @return [Array<Funky::Post>] multiple Funky::Post objects containing data
# fetched by Facebook Graph API.
def posts
posts = Funky::Connection::API.fetch_all("#{id}/posts?fields=type,created_time")
def posts(options = {})

Choose a reason for hiding this comment

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

@kangkyu Tests are missing

@coveralls
Copy link

Coverage Status

Coverage decreased (-13.7%) to 86.308% when pulling 68c1732 on kangkyu:add-since into edbf464 on Fullscreen:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-1.1%) to 85.42% when pulling 259a6ec on kangkyu:add-since into 1f1d930 on Fullscreen:master.

@kangkyu
Copy link
Contributor Author

kangkyu commented Oct 5, 2017

closing in favor of #95

@kangkyu kangkyu closed this Oct 5, 2017
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