-
Notifications
You must be signed in to change notification settings - Fork 17
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 ebirdchecklist() #108
Add ebirdchecklist() #108
Conversation
Thanks for the work on this, @RichardLitt. Yeah I think force pushing can mess up PRs since GitHub may be trying to reference specific previous commits internally in PRs, and force pushing removes those. If you see R CMD check errors in the CI jobs you can often also troubleshoot these locally with |
I think I got it. I didn't know I needed to run |
Let me know if anything needs editing. |
I took this for a spin locally and it's a great start, I have a few suggestions
|
Let me know what you think makes sense for the output. Happy to change as needed. |
Small note: @sebpardo, I'm waiting on you for thoughts on this before moving forward. Let me know what you think! |
Hi @RichardLitt, apologies for the delay in looking into this. I just had played around with it locally and it looks great! I agree with @slager that we need to think about how to return the output of this API call as the call itself returns a complex object. Saying this, I think it can be easily flattened into a data frame even in the special case of a checklist with breeding codes and/or attached media. See the following example: library(dplyr)
cl <- ebirdchecklist("S101576784")
glimpse(cl)
glimpse(cl$subAux)
glimpse(cl$obs)
glimpse(cl$obs$obsAux)
glimpse(cl$obs$mediaCounts)
The nested data frames
I also think we should ignore the columns beginning with With regards to the breeding code, I think the easiest solution would be to have an argument in the function along the lines of Haven't looked deep into the tests yet, but I tried entering two checklist identifiers and the function should throw an error with a more informative message: cl2 <- ebirdchecklist(c("S162668032", "S162668033"))
Lastly, I also agree with @slager in that the documentation needs to provide information on every column being returned. Despite my long response, I don't think it should be too onerous to sort this out! @RichardLitt, let us know if you'd like any help with these suggestions. |
Here's an attempt to wrangle the returned object into a clean output data.frame:
If something like this looks like a good start, I'm happy to put in a PR to ropensci/rebird once Richard's PR gets merged. Note @RichardLitt that @sebpardo bumped the version again on master, so you'll want to merge recent changes to master back into your development branch. |
Thanks, @slager. I think you'd still like me to make the above changes that @sebpardo suggested though, correct? Thanks, @sebpardo, for the long response. One thing:
I'm not sure it's a good practice to remove anything we get from the API in our wrapper. I think we should include it, even if it's potentially wrong, because it's what is gotten from the API and it'd be confusing to users to have that information not be there. I'm happy to implement these changes, but I won't get to it this week, at least. |
@RichardLitt Yes, re: implementing the suggestions from @sebpardo |
I think I solved the issue with #108 (comment) problem 1. @sebpardo, it wasn't clear to me if you wanted to implement all of the changes mentioned in 2, as you started to in #108 (comment). What do you think? |
Great! I will try to find some time to take another look at this soon. |
Thanks much @RichardLitt, merging! Feel free to test and tweak this further, @sebpardo. |
Thank you, @RichardLitt, looks great! |
Thank you, @sebpardo and @slager! I appreciate the extra effort as I waded through this. We're using this endpoint as eBird admins to find checklists that slip past the normal filters, but which shouldn't be in the database due a variety of errors. Hopefully this addition will help us find those easier. Thank you. |
Unfortunately deleted #107 by accident misusing force push, while trying to squash. This is another attempt at fixing #93.