Skip to content
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.

Sign up for GitHub

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

Expose package version info #625

Merged
merged 2 commits into from
Nov 15, 2024
Merged

Expose package version info #625

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5458fdf
Expose package version info
jennybc Nov 13, 2024
78e71dc
Respond to review
jennybc Nov 14, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 26 additions & 0 deletions crates/ark/src/modules/positron/package.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,32 @@
#' @export
.ps.rpc.is_installed <- .ps.is_installed

# Returns a list containing:
# * the version string if the package is installed and NULL otherwise
# * a minimum version to check for, with '0.0.0' as a dummy default
# * a logical indicating if package is installed at or above the minimum version
# This may seem weird, but it's impractical for positron-r to do version
# comparisons.
#' @export
.ps.rpc.packageVersion <- function(pkg, checkAgainst = "0.0.0") {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
.ps.rpc.packageVersion <- function(pkg, checkAgainst = "0.0.0") {
.ps.rpc.packageVersion <- function(pkg, minimumVersion = "0.0.0") {

I'd be tempted to call it this to be super clear that its:

  • a version
  • the minimum one to check against

I'd also be tempted to default it to NULL, implying "no minimum version check is done" - see also rlang::is_installed()

installed <- system.file(package = pkg) != ""

if (installed) {
version <- utils::packageVersion(pkg)
list(
version = as.character(version),
checkAgainst = as.character(checkAgainst),
Copy link
Contributor

Choose a reason for hiding this comment

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

why is this argument returned to the caller?

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess it just felt convenient to package the version checked against and the result together? No more, no less. Maybe you're saying that either the returned info should be completist (include the package name) or be minimal (exclude the version checked against)?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the idea is that the caller knows checkAgainst already, so no need to duplicate the info

Copy link
Contributor

Choose a reason for hiding this comment

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

yep exactly

checkResult = version >= checkAgainst
Copy link
Contributor

Choose a reason for hiding this comment

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

If you move to minimumVersion and drop checkAgainst from the returned value, it could just be

list(version = version/NULL, compatible = TRUE/FALSE)

So you end up with 3 states

# Not installed (caller knows package name, for error reporting purposes)
list(version = NULL, compatible = FALSE)

# Not compatible (caller knows minimum version, for error reporting purposes)
list(version = version, compatible = FALSE)

# Good to go
list(version = version, compatible = TRUE)

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, I'm trying to make argument names and shape match what's on the other side in positron-r. But I don't feel strongly and can just code around whatever form y'all want here.

Copy link
Contributor

Choose a reason for hiding this comment

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

On quick glance it looks like minimumVersion and compatible would also be nice and expressive in https://github.com/posit-dev/positron/pull/5365/files too

In particular I find minimumVersion much easier to understand on first glance than checkAgainst

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah I'm changing it, just haven't pushed yet. Working on both ends.

)
} else {
list(
version = NULL,
checkAgainst = as.character(checkAgainst),
checkResult = FALSE
)
}
}

#' @export
.ps.rpc.install_packages <- function(packages) {
for (pkg in packages) {
Expand Down
Loading