feat: add pagination to search_content()

[toph-allen]

Intent

Add auto-pagination to search_content().

Fixes #447

Approach

• Moved the raw API call to an inner function, which is called with page_offset().

Checklist

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

[toph-allen]

I figured we don't need to expose these to users, since pagination is kind of an implementation detail here. But retaining them and passing them down might make it easier to test performance under different values.

[jonkeane]

But retaining them and passing them down might make it easier to test performance under different values

Yeah, that's true. A few thoughts: (1) one should be able to pass these down in ... even if they are not included in the signature explicitly. (2) what do our other functions do? If they expose these explicitly so that it's obvious there are nobs to tune here, I'm fine including them. But also fine to not (so long as someone can pass them in ...)

[toph-allen]

Our other functions don't expose them to users, and ultimately that's the right choice. I will add a test to ensure they get passed down.

I'll note that passing in page_number will just be the starting page of pagination — pagination will still happen.

[jonkeane]

Cool, sounds good.

I'll note that passing in page_number will just be the starting page of pagination — pagination will still happen.

I'm not sure if this is the right behavior (but we shouldn't fix that here since that's more about the implementation of page_offset). Could you (1) write an issue for this and (2) it would be good to write this down, in a comment at least, but maybe even in the body of the documentation (since we're not explicitly documenting page_number the obvious place is gone, but that's ok even if it's just in the narrative section of the docs, just so someone confused about this and looking would see it).

[toph-allen]

I'm not sure if this is the right behavior (but we shouldn't fix that here since that's more about the implementation of page_offset). Could you (1) write an issue for this and (2) it would be good to write this down, in a comment at least, but maybe even in the body of the documentation (since we're not explicitly documenting page_number the obvious place is gone, but that's ok even if it's just in the narrative section of the docs, just so someone confused about this and looking would see it).

Yeah, this non-intuitiveness was kinda in there in my reasoning for not wanting to include it as an explicit parameter. I'll do both.

[edit] #450

[karawoo]

I disagree on passing the parameters with .... My opinion is that parameters that go through ... should get passed to exported functions with their own documentation. If the only place they're documented is through the ... documentation for this function, to me that just obfuscates them unnecessarily. If we're going to support people providing pagination-related arguments then let's give them informative names that can pop up in tab-completion, etc.

This is just an opinion and doesn't need to block the PR if others feel strongly about the current approach, however.

[jonkeane]

This is great, thanks for something so targeted. I have one suggested improvement, but if you decide not to do that here, that shouldn't hold up the merge.

[jonkeane]

I know this is just an edit, but this is a great example of the power of httptest: we can assert our handling of the responses where that's important, but here we just need to know it's constructing the URLs correctly to know that we're doing the right thing. 💯

[karawoo]

I disagree on passing the parameters with .... My opinion is that parameters that go through ... should get passed to exported functions with their own documentation. If the only place they're documented is through the ... documentation for this function, to me that just obfuscates them unnecessarily. If we're going to support people providing pagination-related arguments then let's give them informative names that can pop up in tab-completion, etc.

This is just an opinion and doesn't need to block the PR if others feel strongly about the current approach, however.

[toph-allen]

I disagree on passing the parameters with .... My opinion is that parameters that go through ... should get passed to exported functions with their own documentation. If the only place they're documented is through the ... documentation for this function, to me that just obfuscates them unnecessarily. If we're going to support people providing pagination-related arguments then let's give them informative names that can pop up in tab-completion, etc.

@karawoo Good point. It does have a bad feel to it.

The other functions that wrap paginated endpoints don't provide pagination controls, and my original intent for this func was to do the same. Added them back in because we may want to use them to test performance of different page_sizes.

Your point makes me feel that including them in ... is probably a bad compromise between my original intent and current functionality. But the behavior of page_number is funky and I have #450 to fix that — I'd like to postpone making the params explicit until that issue.

[edit] I've captured in #450 that the params should be explicit.