-
Notifications
You must be signed in to change notification settings - Fork 26
feat: add get_packages()
and get_content_packages()
functions
#374
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
#' @param name The package name to filter by. | ||
#' @param page_number The page number. | ||
#' @param page_size Page size, max 500. | ||
packages = function(name = NULL, page_number = 1, page_size = 500) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We might have this pattern already, but do we want to keep on explicitly setting and passing all of these args to each endpoint that is paginated?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, the arguments exposed by paginated methods and functions are widespread and consistent in the package.
The way that pagination functions are implemented in connectapi
, the endpoints have to explicitly receive page number and size arguments (or at least page number), because pagination is implemented by using rlang
to create a copy of the quosure, increment the page number in the expression, and call the resulting new quosure (ref).
As far as the user facing side (get_packages()
) — I think with paginated endpoints as currently implemented, having controls over the page size and limit are important for development, because, as with all our paginated endpoints, the packages
endpoint is limited to returning 500 items for pages, so for example calling it on Dogfood takes 7 minutes (I tested in posit-sdk-py
). So when you're developing with it and iterating, it's nice to be able to cut it off after a certain point.
In [14]: timeit(
...: lambda: pd.DataFrame(client.packages.fetch()),
...: number=1
...: )
Out[14]: 437.47203699999955
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should make this so that no user needs to deal with this. Can we wrap all of this so that someone just has to call one thing and they get the data they are looking for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Discussed this with @jonkeane and I want to acknowledge that conversation here: they feel strongly that including access to pagination controls for end users is the wrong choice.
I conditionally disagree — I agree that it'd be much better to hide them, but feel strongly that as long as Connect's endpoints are paginated in a way that makes them deliver data very slowly, we should include controls like limit
to make developing anything with these endpoints at all possible.
For now, I'm going to merge this with the controls in place — I appreciate you letting me trust my gut and experience on this. 🙏🏻
Removing pagination controls should be a priority for APIs where… pagination isn't problematic.
Merging after discussion with @jonkeane a few days ago. It sounds like they assent to merging while disagreeing on an aspect of the approach. |
Intent
Add functions to wrap Connect's packages and content package dependencies APIs.
Not attached to a
connectapi
issue. However, this is in support of Connect documentation work (https://github.com/posit-dev/connect/issues/29920).Approach
Take the simplest possible approach, following existing patterns, for both the server-wide and content-specific package functions, with one exception:
app_id
andapp_guid
, but Connect is moving towardscontent_id
andcontent_guid
fields, so theget_packages()
function only ever returns those newer names.Checklist
NEWS.md
(referencing the connected issue if necessary)?devtools::document()
?