-
Notifications
You must be signed in to change notification settings - Fork 13
Added 'limit' argument to set the maximum number of records to download. #98
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
base: master
Are you sure you want to change the base?
Conversation
…wnload. De default reproduce the previous behavior for back compatibility.
sformel
left a comment
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.
@roliveros-ramos apologies it has taken us a while to respond to this PR. The idea seems sound to me, I have reviewed the code and the implementation works.
@pieterprovoost you can approve this PR, but feel free to review and offer feedback if you would like.
|
@sformel There is a potential problem with this implementation: I believe the total returned by the API is currently exact (I have to double check the API implementation), but it may become an estimate as the database grows. That's why it says |
|
Good points. I've worked with ChatGPT to address these concerns and I think I did it correctly: @roliveros-ramos I can submit this as a new PR, or you could modify yours; whichever you prefer. |
I stepped up into some species with millions of records and no way to control the maximum number of records to download. For an initial exploration in particular, this can be very useful. This pull request adds a new argument to the function 'occurrence': limit, controling the maximum number of records to download. The default value (NULL) reproduce the previous behaviour of the function, downloading all records, so nothing is expected to be broken with this change.