fix parameter in methods

[meztez]

Parameter name limit_chunk does not match chunk in ?crul::Paginator documentation.

library(bcdata)
bec <- bcdc_get_data(bcdc_search("BEC Map")[[1]])

Current call to Paginator$new will complain about parameter limit_chunk being unused.

I check crul commits history, and they changed limit_chunk to chunk 6 months ago.

Great package. Godspeed.

[boshek]

Ok this is great. I think before merging this, we figure out some tests for this.

[meztez]

Testing the function call of the actual method? Let me put a test together.

[boshek]

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.

[meztez]

I'm just adding a test to validate method signature.

[ateucher]

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?

[ateucher]

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...

[boshek]

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.

[ateucher]

Thanks so much @meztez. I'll merge this and we can then add the option and the new test!

[meztez]

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.