Skip to content

feat: Added product environment check helpers #372

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

mconflitti-pbc
Copy link
Contributor

Intent

Fixes #371

Approach

Adds environment check helpers to make it easier to determine where the code is running.

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
Collaborator

@toph-allen toph-allen left a comment

Choose a reason for hiding this comment

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

The functions look good! Since they're public-facing functions, they need documentation and to be exported — see my comments for notes on that and hit me up if you have questions.

Not sure why linting is failing.

Also:

  • After you write documentation you'll need to run devtools::document() to regenerate the documentation.
  • You'll probably get CI failures from the pkgdown site not including some of the functions. The top-level _pkgdown.yml file needs entries for all exported functions. Some of its sections match e.g. all functions that start with get_* but the others will need to be listed manually. I learned this the hard way. 😂

@@ -1,5 +1,9 @@
# connectapi (development version)

## New features

- New functions `get_product()`, `is_local()`, `on_connect()` (updated), and `on_workbench()` help detect the Posit product environment (Posit Connect, Posit Workbench, or local) via environment variables. `get_product()` checks both `POSIT_PRODUCT` and `RSTUDIO_PRODUCT` environment variables. (#371)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since this is the first time on_connect() will be exported, it's actually "new" and not updated.

Suggested change
- New functions `get_product()`, `is_local()`, `on_connect()` (updated), and `on_workbench()` help detect the Posit product environment (Posit Connect, Posit Workbench, or local) via environment variables. `get_product()` checks both `POSIT_PRODUCT` and `RSTUDIO_PRODUCT` environment variables. (#371)
- New functions `get_product()`, `is_local()`, `on_connect()`, and `on_workbench()` help detect the Posit product environment (Posit Connect, Posit Workbench, or local) via environment variables. `get_product()` checks both `POSIT_PRODUCT` and `RSTUDIO_PRODUCT` environment variables. (#371)

@@ -194,8 +194,28 @@ endpoint_does_not_exist <- function(res) {
!("code" %in% names(httr::content(res, as = "parsed")))
}

get_product <- function() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Functions that we wanna export as part of the package namespace (i.e. loaded when running library(connectapi) should have roxygen2 documentation, and that documentation must contain the @export tag. A good pretty light example to look at is the get_runtimes() documentation here. (Except for those docs contain a blank line, which — while it doesn't cause problems — is a mistake.)

roxygen2 docs are preceded by #' and follow this general form (this is the block for get_runtimes().

#' Get available runtimes on server
#'
#' Get a table showing available versions of R, Python, Quarto, and Tensorflow
#' on the Connect server.
#'
#' @param client A `Connect` object.
#' @param runtimes Optional. A character vector of runtimes to include. Must be
#' some combination of `"r"`, `"python"`, `"quarto"`, and `"tensorflow"`. Quarto
#' is only supported on Connect >= 2021.08.0, and Tensorflow is only supported
#' on Connect >= 2024.03.0.
#' @return A tibble with columns for `runtime`, `version`, and `cluster_name`
#' and `image_name`. Cluster name and image name are only meaningful on Connect
#' instances running off-host execution.
#'
#' @examples
#' \dontrun{
#' library(connectapi)
#' client <- connect()
#' get_runtimes(client, runtimes = c("r", "python", "tensorflow"))
#' }
#'
#' @export

These won't need much. In fact, they could be listed on the same documentation page. To do that, you'd follow the pattern with get_jobs() (see here). There, the return values for multiple content items are described i.e. (excerpt):

#' @return
#'
#' - `get_jobs()`: A data frame with a row representing each job.
#' - `get_job_list()`: A list with each element representing a job.

and the end of the documentation includes the @rdname tag

#' @rdname get_jobs
#' @export

Other functions can then include just those same two lines as their documentation to show the same help page when queried (e.g. get_job_list() further down in the file).

That explanation is a little terse — let me know if you have any other questions!

@toph-allen
Copy link
Collaborator

This is why CI is failing: lintr removed the dependency on cyclocomp: https://github.com/r-lib/lintr/releases/tag/v3.2.0

Changing line 25 of .github/workflows/lint.yaml to the following will probably fix:

          extra-packages: local::., any::lintr, any::devtools, any::testthat, any::cyclocomp

@toph-allen
Copy link
Collaborator

(update: you can pull main into this branch to fix linting)

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.

RSTUDIO_PRODUCT is deprecated; support POSIT_PRODUCT
2 participants