Merge branch 'master' into backports-warning

r-lib · Dec 4, 2020 · c653b51 · c653b51
2 parents e9a54d4 + 430d28a
commit c653b51
Show file tree

Hide file tree

Showing 13 changed files with 155 additions and 48 deletions.
diff --git a/.github/workflows/lint-changed-files.yaml b/.github/workflows/lint-changed-files.yaml
@@ -0,0 +1,53 @@
+on:
+  pull_request:
+    branches:
+      - main
+      - master
+
+name: lint-changed-files
+
+jobs:
+  lint-changed-files:
+    runs-on: macOS-latest
+    env:
+      GITHUB_PAT: ${{ secrets.GITHUB_TOKEN }}
+    steps:
+      - uses: actions/checkout@v2
+
+      - uses: r-lib/actions/setup-r@v1
+
+      - name: Query dependencies
+        run: |
+          install.packages('remotes')
+          saveRDS(remotes::dev_package_deps(dependencies = TRUE), ".github/depends.Rds", version = 2)
+          writeLines(sprintf("R-%i.%i", getRversion()$major, getRversion()$minor), ".github/R-version")
+        shell: Rscript {0}
+
+      - name: Cache R packages
+        uses: actions/cache@v2
+        with:
+          path: ${{ env.R_LIBS_USER }}
+          key: ${{ runner.os }}-${{ hashFiles('.github/R-version') }}-1-${{ hashFiles('.github/depends.Rds') }}
+          restore-keys: ${{ runner.os }}-${{ hashFiles('.github/R-version') }}-1-
+
+      - name: Install dependencies
+        run: |
+          install.packages(c("remotes"))
+          remotes::install_deps(dependencies = TRUE)
+          remotes::install_cran("gh")
+          remotes::install_github("jimhester/lintr")
+          remotes::install_cran("purrr")
+        shell: Rscript {0}
+
+      - name: Add lintr options
+        run: cat('\noptions(lintr.linter_file = ".lintr_new")\n', file = "~/.Rprofile", append = TRUE)
+        shell: Rscript {0}
+
+      - name: Extract and lint files changed by this PR
+        run: |
+          files <- gh::gh("GET https://api.github.com/repos/jimhester/lintr/pulls/${{ github.event.pull_request.number }}/files")
+          changed_files <- purrr::map_chr(files, "filename")
+          all_files <- list.files(recursive = TRUE)
+          exclusions_list <- as.list(setdiff(all_files, changed_files))
+          lintr::lint_package(exclusions = exclusions_list)
+        shell: Rscript {0}
diff --git a/.lintr_new b/.lintr_new
@@ -0,0 +1,10 @@
+linters: with_defaults(
+    line_length_linter = line_length_linter(120)
+  )
+exclusions: list(
+  "tests/testthat/dummy_packages",
+  "tests/testthat/knitr_formats",
+  "tests/testthat/knitr_malformed",
+  "inst",
+  "vignettes"
+ )
diff --git a/NEWS.md b/NEWS.md
@@ -1,5 +1,6 @@
 # lintr (development version)
 
+* Added a secondary, more restrictive lint workflow - `lint-changed-files` - for newly written / modified code (#641, @dragosmg) 
 * Switched CI from Travis to GitHub Actions, using the full tidyverse recommended R CMD check. Code coverage and linting are implemented using separate GitHub Actions workflows (#572, @dragosmg)
 * `save_cache` will now recursively create the cache directory; this avoids errors that could arise if any parent directories do not exist (#60, @dankessler).
 * `extract_r_source` handles Rmd containing unevaluated code blocks with named
@@ -16,10 +17,13 @@
 * New `sprintf_linter()` (#544, #578, #624, #625, @renkun-ken, @AshesITR)
 * Exclusions specified in the `.lintr` file are now relative to the location of that file 
   and support excluding entire directories (#158, #438, @AshesITR)
-* `object_name_linter()` now excludes special R hook functions such as `.onLoad` (#500, #614, @AshesITR)
+* `object_name_linter()` now excludes special R hook functions such as `.onLoad` (#500, #614, @AshesITR and @michaelchirico)
 * `equals_na_linter()` now lints `x != NA` and `NA == x`, and skips usages in comments (#545, @michaelchirico)
 * Malformed Rmd files now cause a lint instead of an error (#571, #575, @AshesITR)
 * `object_name_linter()` gains a new default style, `"symbols"`, which won't lint all-symbol object names (in particular, that means operator names like `%+%` are skipped; #615, @michaelchirico)
+* `spaces_inside_linter` ignores spaces preceding trailing comments (#636, @michaelchirico)
+* `T_and_F_symbol_linter` is now part of the default linters (#517, #612, @AshesITR)
+* `with_defaults()` no longer duplicates the `lintr_function` class when it is already present (#511, #612, @AshesITR)
 * New `backport_linter()` for detecting mismatched R version dependencies (#506, @MichaelChirico)
 
 # lintr 2.0.1

diff --git a/R/addins.R b/R/addins.R
@@ -1,3 +1,4 @@
+# nocov start
 addin_lint <- function() {
   filename <- rstudioapi::getSourceEditorContext()
   if (filename$path == "") {
@@ -8,7 +9,7 @@ addin_lint <- function() {
   if (length(config_file) == 0) {
     config_linters <- NULL
   } else {
-    config <- read.dcf(config_file, all = T)
+    config <- read.dcf(config_file, all = TRUE)
     config_linters <- gsub("\n", "", config[["linters"]])
   }
   linters <- if (length(config_linters) == 0) {
@@ -29,3 +30,4 @@ addin_lint_package <- function() {
 
   lintr::lint_package(project_path)
 }
+# nocov end
diff --git a/R/object_name_linters.R b/R/object_name_linters.R
@@ -51,14 +51,14 @@ object_name_linter <- function(styles = c("snake_case", "symbols")) {
     nms <- strip_names(
       as.character(xml2::xml_find_first(assignments, "./text()")))
 
-    generics <- c(
+    generics <- strip_names(c(
       declared_s3_generics(xml),
       namespace_imports()$fun,
       names(.knownS3Generics),
-      .S3PrimitiveGenerics, ls(baseenv()))
+      .S3PrimitiveGenerics, ls(baseenv())))
 
-    style_matches <- lapply(styles, function(x) {
-      check_style(nms, x, generics)
+    style_matches <- lapply(styles, function(style) {
+      check_style(nms, style, generics)
     })
 
     matches_a_style <- Reduce(`|`, style_matches)
@@ -90,6 +90,9 @@ check_style <- function(nms, style, generics = character()) {
       # If they are not conforming, but are S3 methods then ignore them
       conforming[!conforming][has_generic] <- TRUE
     }
+    # exclude namespace hooks like .onLoad, .Last.lib, etc (#500)
+    is_special <- is_special_function(nms[!conforming])
+    conforming[!conforming][is_special] <- TRUE
   }
   conforming
 }
@@ -130,8 +133,7 @@ make_object_linter <- function(fun) {
     keep_indices <- which(
       !is_operator(names) &
         !is_known_generic(names) &
-        !is_base_function(names) &
-        !is_special_function(names)
+        !is_base_function(names)
     )
 
     lapply(
@@ -229,27 +231,34 @@ base_pkgs <- c(
   "mgcv"
 )
 
-base_funs <- unlist(lapply(base_pkgs,
-                           function(x) {
-                             name <- try_silently(getNamespace(x))
-                             if (!inherits(name, "try-error")) {
-                               ls(name, all.names = TRUE)
-                             }
-                           }))
+# some duplicates such as .onLoad appear in multiple packages; sort for efficiency
+base_funs <- sort(unique(unlist(lapply(
+  base_pkgs,
+  function(x) {
+    name <- try_silently(getNamespace(x))
+    if (!inherits(name, "try-error")) {
+      ls(name, all.names = TRUE)
+    }
+  }
+))))
 
 is_base_function <- function(x) {
   x %in% base_funs
 }
 
-# see ?".onLoad" and ?"Startup"
+# see ?".onLoad", ?Startup, and ?quit. Remove leading dot to match behavior of strip_names().
+#   All of .onLoad, .onAttach, and .onUnload are used in base packages,
+#   and should be caught in is_base_function; they're included here for completeness / stability
+#   (they don't strictly _have_ to be defined in base, so could in principle be removed).
+#   .Last.sys and .First.sys are part of base itself, so aren't included here.
 special_funs <- c(
-  ".onLoad",
-  ".onAttach",
-  ".onUnload",
-  ".onDetach",
-  ".Last.lib",
-  ".First",
-  ".Last"
+  "onLoad",
+  "onAttach",
+  "onUnload",
+  "onDetach",
+  "Last.lib",
+  "First",
+  "Last"
 )
 
 is_special_function <- function(x) {
@@ -266,7 +275,7 @@ object_lint <- function(source_file, token, message, type) {
     line = source_file$lines[as.character(token$line1)],
     ranges = list(c(token$col1, token$col2)),
     linter = type
-    )
+  )
 }
 
 

diff --git a/R/spaces_inside_linter.R b/R/spaces_inside_linter.R
@@ -9,7 +9,10 @@ spaces_inside_linter <- function(source_file) {
     "']'")
 
   # using a regex here as checking each token is horribly slow
-  re <- rex(list(one_of("[("), " ") %or% list(" " %if_prev_isnt% ",", one_of("])")))
+  re <- rex(
+    list(one_of("[("), one_or_more(" "), none_of(end %or% "#" %or% " ")) %or%
+      list(" " %if_prev_isnt% ",", one_of("])"))
+  )
   all_matches <- re_matches(source_file$lines, re, global = TRUE, locations = TRUE)
   line_numbers <- as.integer(names(source_file$lines))
 

diff --git a/R/zzz.R b/R/zzz.R
@@ -60,7 +60,7 @@ with_defaults <- function(..., default = default_linters) {
 
   res[] <- lapply(res, function(x) {
     prev_class <- class(x)
-    if (inherits(x, "function")) {
+    if (inherits(x, "function") && !inherits(x, "lintr_function")) {
       class(x) <- c(prev_class, "lintr_function")
     }
     x
@@ -71,7 +71,8 @@ with_defaults <- function(..., default = default_linters) {
 #'
 #' List of default linters for \code{\link{lint}}. Use \code{\link{with_defaults}} to customize it.
 #' @export
-default_linters <- with_defaults(default = list(),
+default_linters <- with_defaults(
+  default = list(),
   assignment_linter,
   closed_curly_linter(),
   commas_linter,
@@ -92,8 +93,10 @@ default_linters <- with_defaults(default = list(),
   single_quotes_linter,
   spaces_inside_linter,
   spaces_left_parentheses_linter,
+  T_and_F_symbol_linter,
   trailing_blank_lines_linter,
-  trailing_whitespace_linter)
+  trailing_whitespace_linter
+)
 
 #' Default undesirable functions and operators
 #'
@@ -103,7 +106,8 @@ default_linters <- with_defaults(default = list(),
 #' @format A named list of character strings.
 #' @rdname default_undesirable_functions
 #' @export
-all_undesirable_functions <- with_defaults(default = list(),
+all_undesirable_functions <- with_defaults(
+  default = list(),
   "attach" = "use roxygen2's @importFrom statement in packages, or `::` in scripts",
   "detach" = "use roxygen2's @importFrom statement in packages, or `::` in scripts",
   "ifelse" = "use an if () {} else {} block",
@@ -126,7 +130,8 @@ all_undesirable_functions <- with_defaults(default = list(),
 
 #' @rdname default_undesirable_functions
 #' @export
-default_undesirable_functions <- do.call(with_defaults, c(list(default=list()),
+default_undesirable_functions <- do.call(with_defaults, c(
+  list(default = list()),
   all_undesirable_functions[c(
     "attach",
     "detach",
@@ -147,15 +152,17 @@ default_undesirable_functions <- do.call(with_defaults, c(list(default=list()),
 
 #' @rdname default_undesirable_functions
 #' @export
-all_undesirable_operators <- with_defaults(default = list(),
+all_undesirable_operators <- with_defaults(
+  default = list(),
   ":::" = NA,
   "<<-" = NA,
   "->>" = NA
 )
 
 #' @rdname default_undesirable_functions
 #' @export
-default_undesirable_operators <- do.call(with_defaults, c(list(default=list()),
+default_undesirable_operators <- do.call(with_defaults, c(
+  list(default = list()),
   all_undesirable_operators[c(
     ":::",
     "<<-",
@@ -174,19 +181,19 @@ settings <- NULL
 # nocov start
 .onLoad <- function(libname, pkgname) {
   op <- options()
-  op.lintr <- list(
+  op_lintr <- list(
     lintr.linter_file = ".lintr"
   )
-  toset <- !(names(op.lintr) %in% names(op))
-  if (any(toset)) options(op.lintr[toset])
+  toset <- !(names(op_lintr) %in% names(op))
+  if (any(toset)) options(op_lintr[toset])
 
   default_settings <<- list(
     linters = default_linters,
     exclude = rex::rex("#", any_spaces, "nolint"),
     exclude_start = rex::rex("#", any_spaces, "nolint start"),
     exclude_end = rex::rex("#", any_spaces, "nolint end"),
     exclusions = list(),
-    cache_directory = "~/.R/lintr_cache", # nolint
+    cache_directory = "~/.R/lintr_cache",
     comment_token = Sys.getenv("GITHUB_TOKEN", unset = NA) %||% rot(
       paste0(
         "0n12nn72507",

diff --git a/tests/testthat/default_linter_testcode.R b/tests/testthat/default_linter_testcode.R
@@ -18,26 +18,28 @@ f = function (x,y = 1){}
 # object_name
 # object_usage
 # open_curly
+# T_and_F_symbol
 someComplicatedFunctionWithALongCamelCaseName <- function(x)
 {
   y <- 1
-	if (1 > 2 && 2 > 3 && 3 > 4 && 4 > 5 && 5*10 > 6 && x == NA) {TRUE} else {FALSE}
+  if (1 > 2 && 2 > 3 && 3 > 4 && 4 > 5 && 5*10 > 6 && 5 > 6 && 6 > 7 && x == NA) {T} else {F}
 }
 
+# no_tab
 # pipe_continuation
 # seq_linter
 # spaces_inside
 x <- 1:10
 x[ 2]
 1:length(x) %>% lapply(function(x) x*2) %>%
-  head()
+	head()
 
 # single_quotes
 message('single_quotes')
 
 # spaces_left_parentheses
 # trailing_whitespace
-y <- 2 +(1:10)    
+y <- 2 +(1:10) 
 
 # trailing_blank_lines
 
diff --git a/tests/testthat/dummy_packages/package/R/default_linter_testcode.R b/tests/testthat/dummy_packages/package/R/default_linter_testcode.R
@@ -18,26 +18,28 @@ f = function (x,y = 1){}
 # object_name
 # object_usage
 # open_curly
+# T_and_F_symbol
 someComplicatedFunctionWithALongCamelCaseName <- function(x)
 {
   y <- 1
-  if (1 > 2 && 2 > 3 && 3 > 4 && 4 > 5 && 5*10 > 6 && x == NA) {TRUE} else {FALSE}
+  if (1 > 2 && 2 > 3 && 3 > 4 && 4 > 5 && 5*10 > 6 && 5 > 6 && 6 > 7 && x == NA) {T} else {F}
 }
 
+# no_tab
 # pipe_continuation
 # seq_linter
 # spaces_inside
 x <- 1:10
 x[ 2]
 1:length(x) %>% lapply(function(x) x*2) %>%
-  head()
+	head()
 
 # single_quotes
 message('single_quotes')
 
 # spaces_left_parentheses
 # trailing_whitespace
-y <- 2 +(1:10)
+y <- 2 +(1:10) 
 
 # trailing_blank_lines