From d8356b7931abc1874032cc02a74267ff444bfd9b Mon Sep 17 00:00:00 2001 From: Aron Atkins Date: Thu, 5 Jan 2023 10:18:56 -0500 Subject: [PATCH 1/2] initial lintr configuration runs lintr during CI. most existing lintr warnings are suppressed rather than reacting to them. --- .Rbuildignore | 1 + .github/workflows/lint.yaml | 32 ++++++++++++++++++++++++++++++++ .lintr | 25 +++++++++++++++++++++++++ R/augment-rprofile.R | 1 - R/available-updates.R | 4 ++-- R/bitbucket.R | 2 +- R/bundle.R | 2 +- R/dependencies.R | 4 ++-- R/external.R | 2 +- R/get-package-actions.R | 1 - R/gitlab.R | 1 - R/install.R | 2 +- R/lockfile-metadata.R | 2 -- R/lockfile.R | 2 +- R/packrat.R | 2 +- R/pkg.R | 6 +++--- R/pretty-print.R | 2 +- R/restore.R | 11 +++++------ R/search-path.R | 4 ++-- R/utils.R | 2 +- tests/testthat/test-hash.R | 7 ++++++- tests/testthat/test-packrat.R | 4 ++-- tests/testthat/test-shiny.R | 3 +-- 23 files changed, 89 insertions(+), 33 deletions(-) create mode 100644 .github/workflows/lint.yaml create mode 100644 .lintr diff --git a/.Rbuildignore b/.Rbuildignore index d6cc33c7..8aee26dd 100644 --- a/.Rbuildignore +++ b/.Rbuildignore @@ -9,6 +9,7 @@ ^RELEASE\.md$ ^\.Rprofile$ ^\.github$ +^\.lintr$ ^tags$ ^R/tags$ ^tests/testthat/packrat/lib$ diff --git a/.github/workflows/lint.yaml b/.github/workflows/lint.yaml new file mode 100644 index 00000000..f4c4ef2d --- /dev/null +++ b/.github/workflows/lint.yaml @@ -0,0 +1,32 @@ +# Workflow derived from https://github.com/r-lib/actions/tree/v2/examples +# Need help debugging build failures? Start at https://github.com/r-lib/actions#where-to-find-help +on: + push: + branches: [main, master] + pull_request: + branches: [main, master] + +name: lint + +jobs: + lint: + runs-on: ubuntu-latest + env: + GITHUB_PAT: ${{ secrets.GITHUB_TOKEN }} + steps: + - uses: actions/checkout@v3 + + - uses: r-lib/actions/setup-r@v2 + with: + use-public-rspm: true + + - uses: r-lib/actions/setup-r-dependencies@v2 + with: + extra-packages: any::lintr, local::. + needs: lint + + - name: Lint + run: lintr::lint_package() + shell: Rscript {0} + env: + LINTR_ERROR_ON_LINT: true diff --git a/.lintr b/.lintr new file mode 100644 index 00000000..e9a59a70 --- /dev/null +++ b/.lintr @@ -0,0 +1,25 @@ +linters: linters_with_defaults( + # some tests have _VERY_ long expectations. + line_length_linter(270), + object_length_linter(80), + object_name_linter = NULL, + object_usage_linter = NULL, + assignment_linter = NULL, + brace_linter = NULL, + commented_code_linter = NULL, + cyclocomp_linter = NULL, + single_quotes_linter = NULL, + seq_linter = NULL + ) +exclusions: list( + # renv is vendored code, not ours. + "inst/resources/renv.R", + # test resources that are not "source". + "tests/testthat/lockfiles", + "tests/testthat/other-packages", + "tests/testthat/packrat", + "tests/testthat/projects", + "tests/testthat/repo", + "tests/testthat/repo-empty", + "tests/testthat/resources" + ) diff --git a/R/augment-rprofile.R b/R/augment-rprofile.R index f75fbd6a..cc20a992 100644 --- a/R/augment-rprofile.R +++ b/R/augment-rprofile.R @@ -47,4 +47,3 @@ editRprofileAutoloader <- function(project, action = c("update", "remove")) { invisible() } - diff --git a/R/available-updates.R b/R/available-updates.R index 6bff1dfe..4f909b9c 100644 --- a/R/available-updates.R +++ b/R/available-updates.R @@ -87,7 +87,7 @@ bitbucketUpdates <- function(lib.loc = .libPaths()) { }) names(DESCRIPTIONS) <- pkgs DESCRIPTIONS <- - Filter(function(x) "RemoteType" %in% colnames(x) && x[,"RemoteType"] == "bitbucket", DESCRIPTIONS) + Filter(function(x) "RemoteType" %in% colnames(x) && x[, "RemoteType"] == "bitbucket", DESCRIPTIONS) if (!length(DESCRIPTIONS)) return(NULL) if (!requireNamespace("httr")) stop("Need package 'httr' to check for Bitbucket updates") do.call(rbind, enumerate(DESCRIPTIONS, function(x) { @@ -152,7 +152,7 @@ gitlabUpdates <- function(lib.loc = .libPaths()) { names(DESCRIPTIONS) <- pkgs DESCRIPTIONS <- Filter(function(x) "RemoteType" %in% colnames(x) && - x[,"RemoteType"] == "gitlab", DESCRIPTIONS) + x[, "RemoteType"] == "gitlab", DESCRIPTIONS) if (!length(DESCRIPTIONS)) return(NULL) if (!requireNamespace("httr")) stop("Need package 'httr' to check for Gitlab updates") do.call(rbind, enumerate(DESCRIPTIONS, function(x) { diff --git a/R/bitbucket.R b/R/bitbucket.R index 569c68ee..f9cd7f26 100644 --- a/R/bitbucket.R +++ b/R/bitbucket.R @@ -33,7 +33,7 @@ bitbucketDownloadHttr <- function(url, destfile, ...) { user <- bitbucket_user(quiet = TRUE) pwd <- bitbucket_pwd(quiet = TRUE) - auth <- if (!is.null(user) & !is.null(pwd)) { + auth <- if (!is.null(user) && !is.null(pwd)) { authenticate(user, pwd, type = "basic") } else { list() diff --git a/R/bundle.R b/R/bundle.R index ad79328d..8f78f3fa 100644 --- a/R/bundle.R +++ b/R/bundle.R @@ -6,7 +6,7 @@ #' be unbundled either with \code{packrat::\link{unbundle}} (which #' restores the project as well), \R's own \code{utils::\link{untar}}, or #' through most system \code{tar} implementations. -#' +#' #' The tar binary is selected using the same heuristic as \code{\link{restore}}. #' #' @param project The project directory. Defaults to the currently activate diff --git a/R/dependencies.R b/R/dependencies.R index 915899a9..e0915097 100644 --- a/R/dependencies.R +++ b/R/dependencies.R @@ -221,7 +221,7 @@ dirDependenciesBuiltIn <- function(dir) { "(?:^", paste0( ignoredDir, - collapse=")|(?:^" + collapse = ")|(?:^" ), ")" ) @@ -828,7 +828,7 @@ fileDependencies.tangle <- function(file, encoding = "UTF-8") { # parse each r chunk independently to retrieve dependencies # allows for some chunks to be _broken_ but not stop retrieving dependencies r_chunks <- strsplit(paste0(readLines(outfile), collapse = "\n"), key)[[1]] - for(r_chunk in r_chunks) { + for (r_chunk in r_chunks) { try(silent = TRUE, { parsed <- parse(text = r_chunk, encoding = encoding) deps <- c(deps, expressionDependencies(parsed)) diff --git a/R/external.R b/R/external.R index 8beafa01..b31f1fb4 100644 --- a/R/external.R +++ b/R/external.R @@ -100,7 +100,7 @@ extlib <- function(packages) { loadExternalPackages <- function() { pkgs <- get_opts("external.packages") if (length(pkgs)) { - pkgs <- pkgs[ !is.null(pkgs) & !is.na(pkgs) & nchar(pkgs) ] + pkgs <- pkgs[!is.null(pkgs) & !is.na(pkgs) & nchar(pkgs)] failures <- dropNull(lapply(pkgs, function(pkg) { tryCatch( expr = extlib(pkg), diff --git a/R/get-package-actions.R b/R/get-package-actions.R index 57a2e471..1f2d017b 100644 --- a/R/get-package-actions.R +++ b/R/get-package-actions.R @@ -101,4 +101,3 @@ packageActionMessages <- function(verb, records) { } msgs } - diff --git a/R/gitlab.R b/R/gitlab.R index 037fe280..92692c9a 100644 --- a/R/gitlab.R +++ b/R/gitlab.R @@ -99,4 +99,3 @@ gitlab_pat <- function(quiet = TRUE) { } return(NULL) } - diff --git a/R/install.R b/R/install.R index 7f19558e..29286a13 100644 --- a/R/install.R +++ b/R/install.R @@ -500,7 +500,7 @@ scan_registry_for_rtools <- function(debug = FALSE) { for (i in seq_along(keys)) { version <- names(keys)[[i]] key <- keys[[version]] - if (!is.list(key) || is.null(key$InstallPath)) next; + if (!is.list(key) || is.null(key$InstallPath)) next install_path <- normalizePath(key$InstallPath, mustWork = FALSE, winslash = "/") diff --git a/R/lockfile-metadata.R b/R/lockfile-metadata.R index c0784101..95dd2192 100644 --- a/R/lockfile-metadata.R +++ b/R/lockfile-metadata.R @@ -102,5 +102,3 @@ available_metadata <- c( r_version = "RVersion", repos = "Repos" ) - - diff --git a/R/lockfile.R b/R/lockfile.R index 857f013c..73f830ac 100644 --- a/R/lockfile.R +++ b/R/lockfile.R @@ -235,7 +235,7 @@ deserializePackages <- function(df) { df <- df[, names(df) != 'requires', drop = FALSE] sortedPackages <- lapply(topoSorted, function(pkgName) { - pkg <- as.list(df[df$name == pkgName,]) + pkg <- as.list(df[df$name == pkgName, ]) pkg <- pkg[!is.na(pkg)] return(pkg) }) diff --git a/R/packrat.R b/R/packrat.R index 74a46995..4527852c 100644 --- a/R/packrat.R +++ b/R/packrat.R @@ -601,7 +601,7 @@ packify <- function(project = NULL, quiet = FALSE) { invisible() } -lockInfo <- function(project, property='packages', fatal=TRUE) { +lockInfo <- function(project, property = 'packages', fatal = TRUE) { project <- getProjectDir(project) diff --git a/R/pkg.R b/R/pkg.R index 858c4c88..d18276b7 100644 --- a/R/pkg.R +++ b/R/pkg.R @@ -133,7 +133,7 @@ getPackageRecordsExternalSource <- function(pkgNames, # Construct the package record by hand -- generate the minimal # bits of the DESCRIPTION file, and infer the package record # from that. - pkg <- available[pkgName,] + pkg <- available[pkgName, ] df <- data.frame( Package = pkg[["Package"]], Version = pkg[["Version"]], @@ -319,7 +319,7 @@ getPackageRecords <- function(pkgNames, # Return TRUE when the data frame for this package has the given RemoteType. hasRemoteType <- function(df, remoteType) { # Do not compare with 'identical'; RemoteType may be a factor. - return (!is.null(df$RemoteType) && df$RemoteType == remoteType) + return(!is.null(df$RemoteType) && df$RemoteType == remoteType) } # Reads a description file and attempts to infer where the package came from. @@ -577,7 +577,7 @@ diffableRecord <- function(record) { # debug helper to print a package record. includes field names, type of value, and value. printPackageRecord <- function(name, record) { cat(name, "\n") - cat(paste(names(record), lapply(record,typeof), record, sep = ":", collapse = "\n"),"\n") + cat(paste(names(record), lapply(record, typeof), record, sep = ":", collapse = "\n"), "\n") } # states: NA (unchanged), remove, add, upgrade, downgrade, crossgrade diff --git a/R/pretty-print.R b/R/pretty-print.R index faa2a245..80a93c58 100644 --- a/R/pretty-print.R +++ b/R/pretty-print.R @@ -62,7 +62,7 @@ prettyPrintPair <- function(packagesFrom, packagesTo, header, footer = NULL, } for (i in seq_along(packagesFrom)) { if (!is.null(packagesFrom[[i]]) && !is.null(packagesTo[[i]])) { - if (!identical(packagesFrom[[i]]$name , packagesTo[[i]]$name)) { + if (!identical(packagesFrom[[i]]$name, packagesTo[[i]]$name)) { stop('Invalid arguments--package names did not match') } } diff --git a/R/restore.R b/R/restore.R index 192c6d17..42048eaa 100644 --- a/R/restore.R +++ b/R/restore.R @@ -35,7 +35,7 @@ isFromCranlikeRepo <- function(pkgRecord, repos) { # the package version is current. NB: Assumes package is present in db. versionMatchesDb <- function(pkgRecord, db) { versionMatch <- - identical(pkgRecord$version, db[pkgRecord$name,"Version"]) + identical(pkgRecord$version, db[pkgRecord$name, "Version"]) # For GitHub, Bitbucket, and Gitlab, we also need to check that the SHA1 is identical # (the source may be updated even if the version hasn't been bumped) @@ -189,8 +189,8 @@ getSourceForPkgRecord <- function(pkgRecord, # If the file wasn't saved to the destination directory (which can happen # if the repo is local--see documentation in download.packages), copy it # there now - if (!identical(fileLoc[1,2], file.path(pkgSrcDir, pkgSrcFile))) { - file.copy(fileLoc[1,2], pkgSrcDir) + if (!identical(fileLoc[1, 2], file.path(pkgSrcDir, pkgSrcFile))) { + file.copy(fileLoc[1, 2], pkgSrcDir) } type <- paste(type, "current") } else { @@ -439,7 +439,7 @@ annotatePkgs <- function(pkgNames, project, lib = libDir(project)) { # Takes a vector of package names, and returns a logical vector that indicates # whether the package was not installed by packrat. -installedByPackrat <- function(pkgNames, lib.loc, default=NA) { +installedByPackrat <- function(pkgNames, lib.loc, default = NA) { # Can't use installed.packages(fields='InstallAgent') here because it uses # Meta/package.rds, not the DESCRIPTION file, and we only record this info in # the DESCRIPTION file. @@ -678,7 +678,7 @@ playActions <- function(pkgRecords, actions, repos, project, lib) { # Changing package type or version: Remove the old one now (we'll write # a new one in a moment) message("Replacing ", pkgRecord$name, " (", action, " ", - installedPkgs[pkgRecord$name,"Version"], " to ", + installedPkgs[pkgRecord$name, "Version"], " to ", pkgRecord$version, ") ... ", appendLF = FALSE) removePkgs(project, pkgRecord$name, lib) } else if (identical(action, "add")) { @@ -1009,4 +1009,3 @@ appendRemoteInfoToDescription <- function(src, dest, remote_info) { return(TRUE) } - diff --git a/R/search-path.R b/R/search-path.R index b102fe27..f96db17c 100644 --- a/R/search-path.R +++ b/R/search-path.R @@ -16,7 +16,7 @@ search_path <- function() { ) ## Filter to only actual packages - pkgs <- pkgs[ grep("^package:", pkgs$package), , drop = FALSE] + pkgs <- pkgs[grep("^package:", pkgs$package), , drop = FALSE] ## Clean up the package name by removing the initial 'package:' pkgs$package <- sub("^package:", "", pkgs$package) @@ -37,7 +37,7 @@ search_path <- function() { pkgs$lib.dir <- dirname(pkgs$lib.loc) ## Arrange by lib.dir - pkgs <- pkgs[ order(pkgs$lib.dir), ] + pkgs <- pkgs[order(pkgs$lib.dir), ] ## Unset the rownames rownames(pkgs) <- NULL diff --git a/R/utils.R b/R/utils.R index 03c85333..0fb0a0fb 100644 --- a/R/utils.R +++ b/R/utils.R @@ -95,7 +95,7 @@ dir_copy <- function(from, to, overwrite = FALSE, all.files = TRUE, if (overwrite) { unlink(to, recursive = TRUE) } else { - stop(paste( sep = "", + stop(paste(sep = "", if (is_dir(to)) "Directory" else "File", " already exists at path '", to, "'." )) diff --git a/tests/testthat/test-hash.R b/tests/testthat/test-hash.R index 39d6ac72..45c78f7a 100644 --- a/tests/testthat/test-hash.R +++ b/tests/testthat/test-hash.R @@ -14,9 +14,14 @@ test_that("hash function is available and has expected arguments", { # which expects this non-exported function to be present. That's not to say # the function signature can't be changed, only that the relevant call in # Connect's packrat_restore.R should be studied to avoid any breakage. + + expect_identical( formals(hash), - pairlist(path = quote(expr =), descLookup = as.name("installedDescLookup")) + pairlist( + path = quote(expr =), # nolint: infix_spaces_linter. r-lib/lintr#1889 + descLookup = as.name("installedDescLookup") + ) ) }) diff --git a/tests/testthat/test-packrat.R b/tests/testthat/test-packrat.R index e92e7ac2..2f6fd711 100644 --- a/tests/testthat/test-packrat.R +++ b/tests/testthat/test-packrat.R @@ -28,7 +28,7 @@ withTestContext({ skip_on_cran() projRoot <- cloneTestProject("sated") init(enter = FALSE, projRoot, options = list(local.repos = "packages"), - infer.dependencies=FALSE) + infer.dependencies = FALSE) lib <- libDir(projRoot) expect_true(file.exists(lockFilePath(projRoot))) expect_true(file.exists(srcDir(projRoot))) @@ -137,7 +137,7 @@ withTestContext({ skip_on_cran() projRoot <- cloneTestProject("partlyignored") lib <- libDir(projRoot) - init(enter = FALSE, projRoot, options = list(ignored.directories="ignoreme")) + init(enter = FALSE, projRoot, options = list(ignored.directories = "ignoreme")) # This test project has a file called notignored.R that depends on bread, and # another file called ignoreme/ignorethis.R that depends on toast. diff --git a/tests/testthat/test-shiny.R b/tests/testthat/test-shiny.R index c5ae860d..0ef139e1 100644 --- a/tests/testthat/test-shiny.R +++ b/tests/testthat/test-shiny.R @@ -1,7 +1,7 @@ test_that("Shiny examples have a shiny dependency", { skip_on_cran() skip_if_not_installed("shiny") - + # Confirm packrat believes all example shiny apps are, in fact, shiny apps examplesPath <- system.file("examples", package = "shiny") apps <- list.files(examplesPath, full.names = TRUE) @@ -17,5 +17,4 @@ test_that("projects which use shiny implicitly are detected", { # R Markdown document with 'runtime: shiny' interactiveDocPath <- file.path("resources", "interactive-doc-example.Rmd") expect_true("shiny" %in% packrat:::fileDependencies(interactiveDocPath)) - }) From 267b9ed364ace01380365e782496aa87e45f2bb9 Mon Sep 17 00:00:00 2001 From: Aron Atkins Date: Fri, 6 Jan 2023 10:05:15 -0500 Subject: [PATCH 2/2] enable the assignment linter --- .lintr | 1 - R/aaa-globals.R | 20 ++++++++++---------- R/hooks.R | 4 ++-- R/rstudio-protocol.R | 2 +- 4 files changed, 13 insertions(+), 14 deletions(-) diff --git a/.lintr b/.lintr index e9a59a70..a9578238 100644 --- a/.lintr +++ b/.lintr @@ -4,7 +4,6 @@ linters: linters_with_defaults( object_length_linter(80), object_name_linter = NULL, object_usage_linter = NULL, - assignment_linter = NULL, brace_linter = NULL, commented_code_linter = NULL, cyclocomp_linter = NULL, diff --git a/R/aaa-globals.R b/R/aaa-globals.R index bc0f5f52..bf88baa6 100644 --- a/R/aaa-globals.R +++ b/R/aaa-globals.R @@ -5,32 +5,32 @@ ## Mutable values that might be modified by the user (code borrowed from knitr) # merge elements of y into x with the same names -merge_list = function(x, y) { - x[names(y)] = y +merge_list <- function(x, y) { + x[names(y)] <- y x } -new_defaults = function(value = list()) { - defaults = value +new_defaults <- function(value = list()) { + defaults <- value - get = function(name, default = FALSE, drop = TRUE) { - if (default) defaults = value # this is only a local version + get <- function(name, default = FALSE, drop = TRUE) { + if (default) defaults <- value # this is only a local version if (missing(name)) defaults else { if (drop && length(name) == 1) defaults[[name]] else { setNames(defaults[name], name) } } } - set = function(...) { - dots = list(...) + set <- function(...) { + dots <- list(...) if (length(dots) == 0) return() if (is.null(names(dots)) && length(dots) == 1 && is.list(dots[[1]])) if (length(dots <- dots[[1]]) == 0) return() defaults <<- merge(dots) invisible(NULL) } - merge = function(values) merge_list(defaults, values) - restore = function(target = value) defaults <<- target + merge <- function(values) merge_list(defaults, values) + restore <- function(target = value) defaults <<- target list(get = get, set = set, merge = merge, restore = restore) } diff --git a/R/hooks.R b/R/hooks.R index 410c11c7..6891d491 100644 --- a/R/hooks.R +++ b/R/hooks.R @@ -17,9 +17,9 @@ snapshotHook <- function(expr, value, ok, visible) { project <- .packrat_mutables$get("project") if (is.null(project)) { - file = "" ## to stdout + file <- "" ## to stdout } else { - file = file.path(project, "packrat", "packrat.log") + file <- file.path(project, "packrat", "packrat.log") } if (inherits(e, "simpleError")) { diff --git a/R/rstudio-protocol.R b/R/rstudio-protocol.R index b175710e..38a0d92d 100644 --- a/R/rstudio-protocol.R +++ b/R/rstudio-protocol.R @@ -12,4 +12,4 @@ # the protocol version number, is used to determine whether the package is too # old to be compatible. -.RStudio_protocol_version = 1 +.RStudio_protocol_version <- 1