Skip to content

Accept integers or characters for hit id column#512

Merged
jonkeane merged 3 commits intomainfrom
hits_id
Mar 5, 2026
Merged

Accept integers or characters for hit id column#512
jonkeane merged 3 commits intomainfrom
hits_id

Conversation

@jonkeane
Copy link
Collaborator

@jonkeane jonkeane commented Mar 4, 2026

Intent

With a change on dev Connect, the hits id column now returns the more correct string type. This PR accepts both types and returns a character always now.

Checklist

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

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR updates the get_usage() function to handle the fact that the Posit Connect API is transitioning the id field in the hits endpoint from an integer type to a string type. The change ensures backward compatibility by coercing integer id values to character strings during parsing, so that both old Connect servers (returning integer IDs) and new Connect servers (returning string IDs) produce the same output type.

Changes:

  • Updated the usage ptype to use NA_character_ instead of NA_integer_ for the id field.
  • Added integer-to-character coercion logic in ensure_column so that integer id values from older Connect servers are automatically cast to character.
  • Updated documentation and tests to reflect the new id type.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
R/ptype.R Changes the id field in the usage ptype from NA_integer_ to NA_character_
R/parse.R Adds coercion logic to convert integer columns to character when the ptype expects character
R/get.R Updates roxygen2 documentation for get_usage() to reflect id as a string
man/get_usage.Rd Auto-generated Rd file reflecting the updated id type documentation
NEWS.md Adds a changelog entry for the type change
tests/testthat/test-parse.R Adds unit tests for the integer-to-character coercion in ensure_column and parse_connectapi_typed
tests/testthat/test-get.R Updates the expected id column type in the integration test from integer to character

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review. Take the survey.

Comment on lines +3 to +4
- `get_usage()` now returns the id column as a character to match other parts of the API.

Copy link

Copilot AI Mar 4, 2026

Choose a reason for hiding this comment

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

Changing the id column type from integer to character is a breaking change for existing users of get_usage() (introduced in 0.8.0) who may be relying on integer comparisons or integer arithmetic on the id column. The other recent releases (0.10.0, 0.11.0) follow the pattern of calling out such changes under a ## Breaking changes subsection. Consider adding a ## Breaking changes subsection to the development version entry noting this type change.

Suggested change
- `get_usage()` now returns the id column as a character to match other parts of the API.
## Breaking changes
- `get_usage()` now returns the `id` column as a character to match other parts of the API. This may break code that relies on `id` being an integer (for example, integer comparisons or arithmetic on `id`).

Copilot uses AI. Check for mistakes.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@karawoo thoughts? My inclination is to leave it where it is, we already marked it as breaking on the connect server, and for most (all?) people this change in types will be entirely inconsequential. But I can add the header if you think it's worth it

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think of this more as a fix than a breaking change because you only noticed this because the ptype stuff threw errors right? And there is no real use for this field otherwise.

Copy link
Collaborator

Choose a reason for hiding this comment

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

we already discussed separately but yeah, I think it's fine where it is.

),
usage = tibble::tibble(
"id" = NA_integer_,
"id" = NA_character_,
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is the id... of the hit? What if we just dropped it? I can't imagine it has any value.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, that would make sense, except that it will have value 🔜 (in the work I'm doing to unify hits + shiny usage we need something to join on, and this is the thing to join on). We can drop it now if we want, though we will need to pull it back when I get through my unification / cleanup work.

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
@jonkeane jonkeane merged commit a8d62d5 into main Mar 5, 2026
22 checks passed
@jonkeane jonkeane deleted the hits_id branch March 5, 2026 19:23
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.

4 participants