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 support in .fetch_results for retrieving results in multiple pages #57

Closed
wants to merge 1 commit into from

Conversation

benubah
Copy link
Collaborator

@benubah benubah commented Jul 1, 2019

This change improves the existing code based on the unexpected behavior reported in the following issue #55

This change improves the existing code based on the unexpected behavior reported in the following issue:  rladies#55
@benubah benubah mentioned this pull request Jul 2, 2019
}


if((length(records) < total_records) & !is.null(res$headers$link)){
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if((length(records) < total_records) & !is.null(res$headers$link)){
if ((length(records) < total_records) & !is.null(res$headers$link)) {

# calculate number of offsets for records above 200
offsetn <- ceiling(total_records/length(records))
all_records <- list(records)
for(i in 1:(offsetn - 1)) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
for(i in 1:(offsetn - 1)) {
for (i in 1:(offsetn - 1)) {

@ledell
Copy link
Member

ledell commented Jul 4, 2019

Hi @benubah I support the changes, except that I got an error when running the code above. Since I needed the updated find_groups() function from #48 to try the code example, I copy-pasted the function in to my shell, so this might have caused an issue, but I ran the code and got an error. Can you double check that this works?

> all_ruser_groups <- find_groups(text = "r-project-for-statistical-computing", fields = "past_event_count, upcoming_event_count, last_event, topics", api_key = meetup_api_key)
Downloading 482 record(s)...Error in as_mapper(.f, ...) : argument ".f" is missing, with no default

I think it makes sense to add these changes in the other PR since they are related to the other PR and required to make that one fully functional (and also makes it easier to test).

@benubah
Copy link
Collaborator Author

benubah commented Jul 4, 2019

@ledell Please see a screenshot of my terminal. It works smoothly for me.
The PR updates .quick_fetch() and .fetch_results(). If you look at the full git diff ec0943b for internals.R from start to finish, few insertions of offset as an argument were made. See what .quick_fetch() now looks like below:
But, I have now added the changes to the other PR #48 to make testing easier.

I did not attach the meetupr package. I just sourced the improved internals.R and find_groups()

.quick_fetch <- function(api_url,
                         api_key = NULL,
                         event_status = NULL,
                         offset = 0,
                         ...) 

image

Feel free to let me know if it still does not work.

@ledell
Copy link
Member

ledell commented Jul 4, 2019

@benubah Thanks, I assume the error I got had to do with me copy/pasting code. I'll try it out on the other PR...

@ledell
Copy link
Member

ledell commented Oct 15, 2020

Hi @benubah can you fix the conflict so we can get this merged? Sorry again for not merging this last year.

@maelle
Copy link
Contributor

maelle commented Jan 11, 2021

👋 @benubah

@maelle
Copy link
Contributor

maelle commented Jan 12, 2021

Given the changes that were made in the default branch, and given that some functions already handle pagination, I think it might be easier to start from scratch/ using code from these functions.

@maelle maelle closed this Jan 12, 2021
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