Conversation
There was a problem hiding this comment.
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
usageptype to useNA_character_instead ofNA_integer_for theidfield. - Added integer-to-character coercion logic in
ensure_columnso that integeridvalues from older Connect servers are automatically cast to character. - Updated documentation and tests to reflect the new
idtype.
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.
| - `get_usage()` now returns the id column as a character to match other parts of the API. | ||
|
|
There was a problem hiding this comment.
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.
| - `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`). |
There was a problem hiding this comment.
@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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
we already discussed separately but yeah, I think it's fine where it is.
| ), | ||
| usage = tibble::tibble( | ||
| "id" = NA_integer_, | ||
| "id" = NA_character_, |
There was a problem hiding this comment.
This is the id... of the hit? What if we just dropped it? I can't imagine it has any value.
There was a problem hiding this comment.
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>
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
NEWS.md(referencing the connected issue if necessary)?devtools::document()?