Closed meztez closed 3 years ago
Ok this is great. I think before merging this, we figure out some tests for this.
Testing the function call of the actual method? Let me put a test together.
I am just concerned that I never wrote any tests that caught this. Ideally something like this is caught in crul
revdep checks particularly since we are depending on this.
I'm just adding a test to validate method signature.
I'm not sure this is quite the test we need, as it's not actually testing any bcdata package code. {crul}
does throw the appropriate error:
library(crul)
con <- HttpClient$new(url = "https://api.crossref.org")
Paginator$new(client = con, limit_param = "rows",
offset_param = "offset", limit = 50, limit_chunk = 10)
#> Error in initialize(...): unused argument (limit_chunk = 10)
The problem is that we obviously aren't testing any code that hits the paginator limit, due to the fact that it would usually mean we'd be testing a big download (which we avoid for good reasons). Maybe a test that temporarily drops the chunk limit really low?
However, the paginator limit will only ever be used when the entire resource is over 10000 features: https://github.com/bcgov/bcdata/blob/master/R/utils-classes.R#L382. Perhaps that should also be tied to the chunk limit (e.g., bcdata.chunk_limit * 10
) or a new option to specify max size before using the paginator? bdata.single_download_limit
? I can't think of a good name for it...
I think bdata.single_download_limit
is a good name. I've always been uncomfortable with that threshold hard coded in there so now is a good time to fix that.
Thanks so much @meztez. I'll merge this and we can then add the option and the new test!
Thanks, I started by using the website form and then finding out this package. I had trouble finding BC boundaries in polygon format. Then this + bcmaps, boom, it is perfect. Makes working in R much easier.
Parameter name
limit_chunk
does not matchchunk
in?crul::Paginator
documentation.Current call to
Paginator$new
will complain about parameterlimit_chunk
being unused.I check
crul
commits history, and they changedlimit_chunk
tochunk
6 months ago.Great package. Godspeed.