-
Notifications
You must be signed in to change notification settings - Fork 12
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
Adds bcdc_list_organizations() & bcdc_list_organization_records() (#322) #323
Conversation
This looks awesome, thanks! The one thing we're not doing here (and in I played around with checking via: if (!organization %in% bcdc_list_organizations()) {
stop("'", organization, "' is not a valid organization", call. = FALSE)
} but it's sloooowwww, especially with groups. I'm not sure it's worth it. Thoughts? |
I am not surprised it is slooooow, given how slow all of the |
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.
Looks great. I regret not thinking of this at the time but the bcdc_list_*
naming convention for something that returns a data.frame is a bit confusing. But we are sorted of locked now which is probably fine.
We could, but the error message that comes back basically just says the url is not found, so we'd be throwing all 404s into one message (i.e., if it 404'd because the catalogue was down people would still get an error telling them that their organization/group doesn't exist) |
I agree. However, I also think that a user mental model where they are primarily thinking about the class of the object returned is probably less frequent than where they are thinking about what content is in the object returned. 🤷♀️ |
We could add a note to the documentation for those fxns "if you supply an invalid group | organization you will get a 404"? I have often thought of adding a general sentence somewhere re: 404s when the catalogue is down, given the frequency we see that in Issues. |
I see |
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.
Thanks so much! One small suggestion then go ahead and merge when you're happy :)
Also sf installation is broken right now - don't worry about that failure on Linux |
Co-authored-by: Andy Teucher <andy.teucher@gmail.com>
bcdc_list_organizations()
bcdc_list_organization_records()
NEWS.md
Closes #322.