Skip to content

Conversation

@sebpardo
Copy link
Contributor

@sebpardo sebpardo commented Jan 24, 2019

Based on @gbabineau 's PR, I noticed it would be easiest to include the /ref/hotspot/info API request into the ebirdregioninfo() function rather than create a standalone function to do so (ebirdhotspotinfo()). The main thing missing from this function is proper error checking and messaging, since it does not used ebird_GET().

@gbabineau , I can give you access to my fork so you can add changes to this branch and PR.

Edit:

To Do list

  • update ebirdregioninfo() to include hotspot info
  • add proper error checking and messaging (might be easiest to just copy the error messaging code from ebird_GET()
  • write updated tests
  • update documentation, README, and bump version

@gbabineau
Copy link
Contributor

gbabineau commented Jan 24, 2019 via email

@sebpardo
Copy link
Contributor Author

No rush, birding comes first! (I was actually in Belize last week, birding there is great!)

I've sent you the invite so you can edit my fork, make sure you make your commits in the hotspotinfo branch, and once you push onto my fork they should appear automatically in this PR. I've also added a task list in the original post to give us both a rough idea what needs to be done before merging.

Have a great trip!

@sebpardo
Copy link
Contributor Author

sebpardo commented Feb 3, 2019

Hi @gbabineau , I've made a number of edits to the function and updated the documentation accordingly. The major difference is that I changed the way in which the json object is turned into a data frame so that the column classes are correct from the beginning, so we don't have to change the classes manually. Please have a look and tell me if you think before I merge this PR.

@gbabineau
Copy link
Contributor

Thanks @sebpardo, this looks good! Sorry I have been busy. I am glad you figured out a way not to keep the column class info. I think the documentation (man pages, vignette) still need to be updated. I am on travel without access to my computer so could not run it but see the tests passed or update the documentation. I can also look at it this weekend when I am home.

@sebpardo sebpardo merged commit b929b5d into ropensci:master Feb 12, 2019
@sebpardo sebpardo deleted the hotspotinfo branch March 26, 2019 12:15
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.

2 participants