`page_number` behavior for all functions that call `page_offset()` is unintuitive.

[toph-allen]

Auto-pagination was added in #449 for search_content(). In this implementation, you can pass in page_number and page_size to search_content() via ... and they'll be passed down to an inner call.

The way things are wired up, page_number affects the starting page, but pagination still happens, and the rest of the results will be fetched. This is a little confusing, so it's documented in a comment and in the docs for search_content()'s ... parameter.

Maybe a more intuitive approach would be for page_number to fetch only the specific page if set, and to paginate everything if not set. We could do this by just directly dispatching an inner_search() call if it's set.

Better to make these parameters explicit, too (see @karawoo's comment here: #449 (comment))?

[jonkeane]

Is this something that is unique to the search endpoint? Or is this something that is about how page_offset() works?

[toph-allen]

It's to do with how page_offset works. My understanding is that, like, it's a function expressly to paginate, and its inner loop doesn't know and doesn't care if it's on page 2 because it incremented from page 1, or because page 2 was passed in explicitly — it just increments and loops.

If I put myself in the place of the person writing it, I can see that approach making sense — like, "Why would I call page_offset if I don't want to iterate through pages?"

[jonkeane]

Ok, I updated the title to reflect that. You might also want to shift the focus of the issue description too so it's clear to future us if/when we pick this up.

[toph-allen]

Oh, I misunderstood your question — I thought you were asking about the technical reason the issue occurs, rather than the scope of what's to be addressed here. I had been thinking of this as a tightly-scoped PR to adjust the behavior of search_content().

There aren't other functions in connectapi that provide a page_number parameter to the user and call page_offset() — other functions abstract away pagination controls entirely. So from that perspective, this is unique to the search_content() function.

[jonkeane]

There aren't other functions in connectapi that provide a page_number parameter to the user and call page_offset() — other functions abstract away pagination controls entirely. So from that perspective, this is unique to the search_content() function.

Ah, looking through the code now, that's right. But partially that's because we don't have pass ... as an expansion point for any of those other functions (yet). If we did pass ... those other calls would have the same issue where page_number behaves quite strangely. Frankly, I'm not sure there's any value at all in exposing that argument (it's probably more confusing than helpful in the off chance you want all the results, but don't care about the first N pages!).

As a way forward: maybe what we should do is actually keep the ... passing pattern while wrapping pagination, but not have page_number be explicit the wrapped function. This does mean that if folks attempt to pass page_number, it will be passed. We could sniff ... + add a warning or even strip it out in the wrapping functions that notes the strange behavior, but I doubt that's worth the added complexity.

[toph-allen]

Ah, looking through the code now, that's right. But partially that's because we don't have pass ... as an expansion point for any of those other functions (yet). If we did pass ... those other calls would have the same issue where page_number behaves quite strangely. Frankly, I'm not sure there's any value at all in exposing that argument (it's probably more confusing than helpful in the off chance you want all the results, but don't care about the first N pages!).

Yeah, I agree that there isn't really any utility in exposing the argument. The argument for exposing them is that it's bad to have ... pass in parameters that could affect the function's behavior and aren't documented as named arguments on another exported function.

As a way forward: maybe what we should do is actually keep the ... passing pattern while wrapping pagination, but not have page_number be explicit the wrapped function. This does mean that if folks attempt to pass page_number, it will be passed. We could sniff ... + add a warning or even strip it out in the wrapping functions that notes the strange behavior, but I doubt that's worth the added complexity.

The way that page_offset() is wired up, page_number needs to be explicit in – page_number still needs to be passed in by the paginator function either way — is there a difference between having page_number as a named argument versus extracted from ... with the inner function? I had thought not, since that function isn't exported. We might be talking at crossed purposes.

I agree that it isn't worth the added complexity to actually strip it out.

[jonkeane]

The argument for exposing them is that it's bad to have ... pass in parameters that could affect the function's behavior and aren't documented as named arguments on another exported function.

This sounds like we are aiming for purity over a user interface that is not surprising or confusing. If we don't think it's good or useful to expose the argument we should not expose the argument. If we are exposing just so we can document it (and effectively tell someone they shouldn't use it, cause it's confusing) that's not good.

[karawoo]

This sounds like we are aiming for purity over a user interface that is not surprising or confusing. If we don't think it's good or useful to expose the argument we should not expose the argument.

I disagree with the first sentence but agree with the second. When I see a function that takes ... I usually expect that the function documentation says what other function ... gets passed to so I can look up the right arguments and how they behave. Right now, it seems like we're using ... just to obfuscate arguments that we're uneasy about. To me, the less confusing options would be either 1) don't expose the arguments at all (via ... or otherwise), or 2) expose them with good defaults and document them clearly.

[toph-allen]

Doing a bit more digging, there's a pattern that's used in a few places where we fetch paginated resources.

• page_number: Not exposed to users, just an implementation detail of the API.
• page_size: Exposed at the top level and documented (though not necessarily in enough detail for a user to understand what it does).
• limit: Defaults to Inf, a parameter to page_* functions, "stop paginating after you fetch this many items".

This feels like a good pattern to maintain to me. (I realize that introducing a new character like limit at this point in the discussion may prompt further opinions!)

One difference is that the inner "direct API mapping" functions are no longer made available, but that's fine by me (in older code they're available as methods on the client object — but if you want to directly call a specific page, you probably know enough to just use Connect$GET? 🤔

[jonkeane]

Right now, it seems like we're using ... just to obfuscate arguments that we're uneasy about.

We should be using ... here for future expansion: if the API grows additional queries they can be included without needing to update connectapi (of course, we might want to promote them to be named, but giving users the ability to use them without updating if they knew is good). IMO for APIs like this where we are closely wrapping the API we should always have ... for this expansion. Even if there are no current arguments that could go there.

When I went to confirm this in the code it looks like that was (accidentally?) removed when we adapted the search function to use paging.

connectapi/R/content.R

Lines 1615 to 1620 in dc55d83

	query <- list(
	q = q,
	page_number = page_number,
	page_size = page_size,
	include = include
	)

should have the ... there and .search_content should accept .... Let's adapt this issue to do this.

when I see a function that takes ... I usually expect that the function documentation says what other function ... gets passed to so I can look up the right arguments and how they behave.

I agree with this generally, yes. But in this case where we are using ... as a place for future expansion we are a bit stuck with this already-extant but confusing argument. IMO leaving it hidden in ... is the least bad option here, though agree that it does go against documenting arguments if they are useful and not having them surfaced if they are not. The alternative that I proposed: we fish the confusing arguments out of the ... here is still possible, but I still think that's not worth the code complexity to do.