-
-
Notifications
You must be signed in to change notification settings - Fork 208
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
Feature/version string #963
Conversation
Codecov Report
@@ Coverage Diff @@
## master #963 +/- ##
==========================================
+ Coverage 82.39% 82.48% +0.09%
==========================================
Files 63 63
Lines 3158 3163 +5
==========================================
+ Hits 2602 2609 +7
+ Misses 556 554 -2
Continue to review full report at Codecov.
|
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.
LGTM other than 2 minor nitpicks.
R/tools.R
Outdated
##' \code{\link{Rcpp.package.skeleton}} | ||
##' @examples getRcppVersion() | ||
getRcppVersion <- function(devel = FALSE) { | ||
rcpp <- .Call("getRcppVersionStrings")[if(devel) 2 else 1] |
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.
nit: use .Call("getRcppVersionStrings", PACKAGE = "Rcpp")
to ensure the symbol gets looked up directly in the Rcpp library?
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.
Sure. Look at all the other .Call()
we do -- it is not too consistent.
R/tools.R
Outdated
##' @examples getRcppVersion() | ||
getRcppVersion <- function(devel = FALSE) { | ||
rcpp <- .Call("getRcppVersionStrings")[if(devel) 2 else 1] | ||
.make_numeric_version(rcpp, |
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.
Why not just use package_version()
here?
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.
Good point. I think I was looking inside that function and inadvertendly copied it, rather than calling it.
@kevinushey All good? |
A simple extension to get the "API" version of the package out.
We have always have
packageVersion("Rcpp")
(or the earlierpackageDescription("Rcpp")$Version
) but as I keep increasing the 'dev version' during development, this leaks eg 1.0.1.2 right now when used by e.g.Rcpp.package.skeleton()
. Which then bites potential users who would only have CRAN's 1.0.1 or older.So I added two new
#define
statements for string versions in the usualconfig.h
, plus a simple function to export to the R side -- where we turn it intopackage_version
objects. Nothing major, but a little help.@kevinushey This may also help for the RStudio package generator.