[MINOR][R] small tidying of sh scripts for R #28419
Conversation
|
Test build #122116 has finished for PR 28419 at commit
|
|
Test build #122125 has finished for PR 28419 at commit
|
| devtools::install_github("jimhester/[email protected]") | ||
| # Installs lintr if needed | ||
| if (!requireNamespace("lintr")) { | ||
| install.packages('lintr') |
There was a problem hiding this comment.
Looks like there was sort of a reason for installing a particular version: #26564 (comment) But the logic seems to be "use the new 2.0.0 version". This would always pick up the latest. I'm guessing that's OK, @dongjoon-hyun ?
There was a problem hiding this comment.
the comment said the CRAN version wasn't recent enough so I assumed >= 2.0.0 would be fine. & we're passing so that's a good sign.
Anyway, yes, would be good to confirm on this.
There was a problem hiding this comment.
Ya. It looks okay since this seems to install lintr 2.0.1 as of today.
|
|
||
| # Generate Rd files if devtools is installed | ||
| "$R_SCRIPT_PATH/Rscript" -e ' if("devtools" %in% rownames(installed.packages())) { library(devtools); setwd("'$FWDIR'"); devtools::document(pkg="./pkg", roclets=c("rd")) }' | ||
| "$R_SCRIPT_PATH/Rscript" -e ' if(requireNamespace("devtools", quietly=TRUE)) { setwd("'$FWDIR'"); devtools::document(pkg="./pkg", roclets="rd") }' |
There was a problem hiding this comment.
It seems fine but I don't know these commands; any potential behavior change here?
There was a problem hiding this comment.
requireNamespace is a more minimal test to be sure devtools:: will work correctly.
This is the more common route to check for available functionality, e.g. for Suggested (not required) packages. this is how the suggested arrow dependency is handled, for example:
grep -r "requireNamespace" R/pkg/R -n
R/pkg/R/types.R:91: if (!requireNamespace("arrow", quietly = TRUE)) {
R/pkg/R/deserialize.R:235: if (requireNamespace("arrow", quietly = TRUE)) {
R/pkg/R/SQLContext.R:152: if (requireNamespace("arrow", quietly = TRUE)) {
R/pkg/R/serialize.R:225: if (requireNamespace("arrow", quietly = TRUE)) {
R/pkg/R/DataFrame.R:1229: if (requireNamespace("arrow", quietly = TRUE)) {
The installed.packages is much more cumbersome -- it looks up the full set of available packages, then searches to see if this particular one is there.
srowen
changed the title
dongjoon-hyun
left a comment
dongjoon-hyun
left a comment
There was a problem hiding this comment.
+1, LGTM. Merged to master. Thank you, @MichaelChirico and @srowen .
4 participants
What changes were proposed in this pull request?
Some tidying of
shscripts inR/Why are the changes needed?
Not strictly needed, but the
'devtools' %in% installed.packages()line in particular is "improper" / proabbly slowDoes this PR introduce any user-facing change?
No
How was this patch tested?
Not