Skip to content

[MINOR][R] small tidying of sh scripts for R #28419

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

Closed
MichaelChirico wants to merge 2 commits into from
Closed

[MINOR][R] small tidying of sh scripts for R #28419

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
fb355ea
small tidying of sh scripts for R
Apr 30, 2020
c396869
update lintr to use CRAN
Apr 30, 2020
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
2 changes: 1 addition & 1 deletion R/create-docs.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ pushd "$FWDIR" > /dev/null
mkdir -p pkg/html
pushd pkg/html

"$R_SCRIPT_PATH/Rscript" -e 'libDir <- "../../lib"; library(SparkR, lib.loc=libDir); library(knitr); knit_rd("SparkR", links = tools::findHTMLlinks(paste(libDir, "SparkR", sep="/")))'
"$R_SCRIPT_PATH/Rscript" -e 'libDir <- "../../lib"; library(SparkR, lib.loc=libDir); knitr::knit_rd("SparkR", links = tools::findHTMLlinks(file.path(libDir, "SparkR")))'

popd

Expand Down
2 changes: 1 addition & 1 deletion R/create-rd.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,4 +34,4 @@ pushd "$FWDIR" > /dev/null
. "$FWDIR/find-r.sh"

# 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") }'
Copy link
Member

@srowen srowen Apr 30, 2020

Choose a reason for hiding this comment

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

It seems fine but I don't know these commands; any potential behavior change here?

Copy link
Contributor Author

@MichaelChirico MichaelChirico Apr 30, 2020

Choose a reason for hiding this comment

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

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.

9 changes: 4 additions & 5 deletions dev/lint-r.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,14 +20,13 @@ SPARK_ROOT_DIR <- as.character(argv[1])
LOCAL_LIB_LOC <- file.path(SPARK_ROOT_DIR, "R", "lib")

# Checks if SparkR is installed in a local directory.
if (! library(SparkR, lib.loc = LOCAL_LIB_LOC, logical.return = TRUE)) {
if (!requireNamespace("SparkR", lib.loc = LOCAL_LIB_LOC)) {
stop("You should install SparkR in a local directory with `R/install-dev.sh`.")
}

# Installs lintr from Github in a local directory.
# NOTE: The CRAN's version is too old to adapt to our rules.
if ("lintr" %in% row.names(installed.packages()) == FALSE) {
devtools::install_github("jimhester/[email protected]")
# Installs lintr if needed
if (!requireNamespace("lintr")) {
install.packages('lintr')
Copy link
Member

@srowen srowen Apr 30, 2020

Choose a reason for hiding this comment

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

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 ?

Copy link
Contributor Author

@MichaelChirico MichaelChirico Apr 30, 2020

Choose a reason for hiding this comment

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

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.

Copy link
Member

@dongjoon-hyun dongjoon-hyun Apr 30, 2020

Choose a reason for hiding this comment

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

Ya. It looks okay since this seems to install lintr 2.0.1 as of today.

}

library(lintr)
Expand Down