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

If there are no events, return empty tibble instead of an error #89

Closed
ledell opened this issue Dec 16, 2020 · 9 comments · Fixed by #92
Closed

If there are no events, return empty tibble instead of an error #89

ledell opened this issue Dec 16, 2020 · 9 comments · Fixed by #92

Comments

@ledell
Copy link
Member

ledell commented Dec 16, 2020

I think it would be more user-friendly for the get_events() function to return an empty (zero rows) tibble instead of an error when there's no results. We could convert the error message into a warning.

I ran into this error when writing a function to get and rbind all R-Ladies events into a single tibble, and my function died in the middle because of this error, rather than just rbinding an empty frame.

Note: This repro will only work if there are still no upcoming events for the Sao Paulo chapter (check here).

events <- get_events(urlname = "rladies-sao-paulo", event_status = "upcoming")

When there are no results, it will give the following error:

Error: Zero records match your filter. Nothing to return.

Right now, we have to wrap in a tryCatch() to be able to use get_events() inside another function w/o potential errors:

events <- tryCatch(get_events(urlname = "rladies-sao-paulo",
                              event_status = "upcoming"),
                   error = function(e) {
                     message("No events found.")
                   })

TO DO: Check if this is the case in other functions and if so, change this ticket to cover all the functions and make the changes all at once.

@maelle
Copy link
Contributor

maelle commented Jan 7, 2021

@Athanasiamo made this a warning

@maelle
Copy link
Contributor

maelle commented Jan 7, 2021

(it's a note for info, I like the idea presented in the first comment)

@maelle maelle mentioned this issue Jan 8, 2021
@maelle
Copy link
Contributor

maelle commented Jan 8, 2021

meetupr::get_events("rladies-nashville")
#> Warning: Zero records match your filter. Nothing to return.

#> Warning: Zero records match your filter. Nothing to return.
#> Downloading 0 record(s)...
#> # A tibble: 0 x 21
#> # … with 21 variables: id <chr>, name <chr>, created <dttm>, status <chr>,
#> #   time <dttm>, local_date <date>, local_time <chr>, waitlist_count <int>,
#> #   yes_rsvp_count <int>, venue_id <int>, venue_name <chr>, venue_lat <dbl>,
#> #   venue_lon <dbl>, venue_address_1 <chr>, venue_city <chr>,
#> #   venue_state <chr>, venue_zip <chr>, venue_country <chr>, description <chr>,
#> #   link <chr>, resource <list>

Created on 2021-01-08 by the reprex package (v0.3.0.9001)

So thanks to @Athanasiamo's work things already work as you wish @ledell. Now, do we want to make the warning disappear and instead have some sort of message based on whether verbose is TRUE?

@ledell
Copy link
Member Author

ledell commented Jan 10, 2021

@maelle @Athanasiamo Hey! Thank you @Athanasiamo!

I am seeing two warnings (on master). It looks slightly different than what @maelle posted above (there's a different order... maybe different versions of R? I am using 3.5.3). Regarding the duplicate warning -- maybe get_events() is calling .quick_fetch() twice internally, so it's printing the error twice?

> meetupr::get_events("rladies-nashville")
Downloading 0 record(s)...# A tibble: 0 x 21
# … with 21 variables: id <chr>, name <chr>, created <dttm>, status <chr>,
#   time <dttm>, local_date <date>, local_time <chr>, waitlist_count <int>,
#   yes_rsvp_count <int>, venue_id <int>, venue_name <chr>, venue_lat <dbl>,
#   venue_lon <dbl>, venue_address_1 <chr>, venue_city <chr>,
#   venue_state <chr>, venue_zip <chr>, venue_country <chr>, description <chr>,
#   link <chr>, resource <list>
Warning messages:
1: Zero records match your filter. Nothing to return.

2: Zero records match your filter. Nothing to return.

@maelle Regarding your question -- I don't have a strong preference. What are the downsides of displaying the warning by default? Is better/more common to keep the warnings silent & only turned on via a verbose arg?

@drmowinckels
Copy link
Member

right, we do call .quick_fetch twice, so that makes sense. When the verbose option is in place, we can silence the first call.

@maelle
Copy link
Contributor

maelle commented Jan 11, 2021

Why do we call .quick_fetch() twice?

I think it makes sense to have the warning be a message because there's nothing the user did wrong, just no result. The users themselves will hopefully have some sort of result assessment in place. I'll work on this later this week.

@malcolmbarrett
Copy link
Contributor

As someone who uses this function a lot where nothing gets returned, I would personally like it if it was not a warning, either. (I already wrap this with purrr since It's common for there to be no events.) IMO, it's not something to throw a warning to the user about; it's a normal condition for the data to be in. Might a message be more appropriate?

@maelle
Copy link
Contributor

maelle commented Jan 11, 2021

Thanks @malcolmbarrett - btw any feedback on the current changes in the package is welcome! We've merged a PR allowing non interactive use, and there's one about automatic rate limiting. Do you use the default app? End of my questions 😉

@malcolmbarrett
Copy link
Contributor

I'll let you know if I do! I don't think I use the default app in my use case. If I remember right, I was able to get a free one in the transition period from API key to OAuth

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants