Skip to content

Conversation

@toph-allen
Copy link
Collaborator

Intent

Adjust parameters in search_content()

  • Expose page_size (query param) and limit (page_offset() arg) in search_content() args following pattern elsewhere in connectapi.
  • Do not expose page_number; note the behavior in ... documentation.
  • Pass down ... and ensure it gets added to the query string.

Fixes #450

Approach

  • Moved parameters around.
  • Adjusted tests; added a test for an arbitrarily named other param.

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 toph-allen requested review from jonkeane and karawoo October 8, 2025 20:02
Comment on lines 500 to 501
# This test also confirms the behavior that page_number is passed down via
# ..., even though this isn't behavior we *require*.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice comment explaining why we're testing this thing and that it's not that we believe that page_number is useful, just that it's something that will be passed down.

An alternative (that honestly might not be better! I'm meh about it) would be to use a random extra named argument here. Since we're just asserting expect_GET we just need to confirm the argument is dutifully passed down. But it would require a similar note "We are calling this with not_a_param which isn't a query parameter in the real API to ensure that we are passing unknown args through ... to the query itself.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We already have such a test on line 525! :)

I guess… I put this separate test in because page_number is a named param in the inner function, rather than something that just gets incidentally passed down in via ....

I'll remove this test. I don't think the other test needs a similar note because the test name makes it clear what it's doing.

  test_that("content search passes arbitrary parameters through ... to query string", {
    without_internet(
      expect_GET(
        search_content(
          client,
          q = "bream",
          future_param = "value"
        ),
        "https://connect.example/__api__/v1/search/content?q=bream&page_number=1&page_size=500&include=owner%2Cvanity_url&future_param=value" #nolint
      )
    )
  })

(Lmk if I misunderstood what you're saying!)

@toph-allen toph-allen requested review from jonkeane and karawoo October 9, 2025 17:48
Copy link
Contributor

@karawoo karawoo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

took us all a minute to get on the same page, but I feel good about this.

@toph-allen toph-allen merged commit 14f7487 into main Oct 9, 2025
21 checks passed
@toph-allen toph-allen deleted the toph/search_content_three_dots branch October 9, 2025 20:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

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

4 participants