Expose package version info #625
Conversation
| version <- utils::packageVersion(pkg) | ||
| list( | ||
| version = as.character(version), | ||
| checkAgainst = as.character(checkAgainst), |
There was a problem hiding this comment.
why is this argument returned to the caller?
There was a problem hiding this comment.
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)?
There was a problem hiding this comment.
I think the idea is that the caller knows checkAgainst already, so no need to duplicate the info
| version <- utils::packageVersion(pkg) | ||
| list( | ||
| version = as.character(version), | ||
| checkAgainst = as.character(checkAgainst), |
There was a problem hiding this comment.
I think the idea is that the caller knows checkAgainst already, so no need to duplicate the info
| # 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") { |
There was a problem hiding this comment.
| .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()
| list( | ||
| version = as.character(version), | ||
| checkAgainst = as.character(checkAgainst), | ||
| checkResult = version >= checkAgainst |
There was a problem hiding this comment.
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)There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Yeah I'm changing it, just haven't pushed yet. Working on both ends.
Required by posit-dev/positron#5365 to address posit-dev/positron#1957
There are times when positron-r needs multiple pieces of related info about a package installation. I know it looks funny to get the package version and to pass back info about whether that version satisfies a minimum version requirement, but it's impractical to do that comparison in positron-r (i.e. R package versions don't play nicely with semver).
And to message about a package that is installed, but at insufficient version, you need all 3 of these pieces of info: installed version, required version, and whether that requirement is met.