grow results by page in page_cursor

[karawoo]

Intent

Fixes #501

Approach

We were appending results iteratively with c() and, while not the slowest part of this code, it was improved by growing the list page by page and then combining once at the end with do.call(). Unfortunately we cannot pre-allocate the list because the API does not return the total number of pages, we have to go through them one by one until we reach the end.

For a get_usage_static() call returning 175k results this shaved off about half a second (~11.5 seconds to just under 11). Not a huge improvement but still worth making since the code is no more complex.

While I was in there I also noticed some incorrect documentation and unnecessary tidy evaluation that I've also removed.

Checklist

• Does this change update NEWS.md (referencing the connected issue if necessary)?
• Does this change need documentation? Have you run devtools::document()?

[jonkeane]

Thanks for this

[jonkeane]

One comment: given your comment on #501 do we want to say that this PR fixes #501? Or just that it helps?

[nealrichardson]

One comment: given your comment on #501 do we want to say that this PR fixes #501? Or just that it helps?

To fix #501, or do more towards it, can we increase the page size (by exposing the arg if it isn't already, and setting its default) to 500, rather than the default from the server of 20? https://docs.posit.co/connect/api/#get-/v1/instrumentation/content/visits

[karawoo]

One comment: given your comment on #501 do we want to say that this PR fixes #501? Or just that it helps?

I think technically the way 501 is written, this closes it. We can improve performance of other functions that will help the ultimate use case (get_usage_static/get_usage_shiny). Improving parse_connectapi_typed is already covered in #383.

To fix #501, or do more towards it, can we increase the page size (by exposing the arg if it isn't already, and setting its default) to 500, rather than the default from the server of 20? https://docs.posit.co/connect/api/#get-/v1/instrumentation/content/visits

@joshyam-k discovered that when this function is called with limit = Inf (or any value >500), the page size gets set to 500:

connectapi/R/connect.R

Line 725 in 1bfa7ea

limit = valid_page_size(limit),

I do think that exposing the page size is a good idea though and will open an issue for that as well. edit: added #506

[nealrichardson]

limit = valid_page_size(limit), 

Looks like that's setting limit, not page size? Maybe I'm missing something but when I traced this through before, I didn't see where a page_size query param was getting made. I could be wrong though.

[karawoo]

Looks like that's setting limit, not page size? Maybe I'm missing something but when I traced this through before, I didn't see where a page_size query param was getting made. I could be wrong though.

Yeah it is very confusing. For the API, limit is the page size. Each API call only returns one page of data, so limit controls the size of that page. It's confusing because in the context of the connectapi argument limit means something different (the total number of records to return).

[nealrichardson]

Oh yeah wow that is confusing! Thanks for straightening me out.