Merge branch 'main' into patch-2

ropensci-review-tools · Sep 26, 2024 · 6137f57 · 6137f57
2 parents 603e775 + e55dfca
commit 6137f57
Show file tree

Hide file tree

Showing 8 changed files with 45 additions and 179 deletions.
diff --git a/R/chk_lintr.R b/R/chk_lintr.R
@@ -52,7 +52,7 @@ CHECKS$lintr_line_length_linter <- make_check(
   }
 )
 
-CHECKS$lintr_trailing_semicolon_linter <- make_check(
+CHECKS$lintr_semicolon_linter <- make_check(
 
   description = "No trailing semicolons",
   tags = c("style", "lintr"),
@@ -63,7 +63,7 @@ CHECKS$lintr_trailing_semicolon_linter <- make_check(
         forbid them",
 
   check = function(state) {
-    get_lintr_state(state, "trailing_semicolon_linter")
+    get_lintr_state(state, "semicolon_linter")
   }
 )
 

diff --git a/R/chk_namespace.R b/R/chk_namespace.R
@@ -8,8 +8,8 @@ CHECKS$no_import_package_as_a_whole <- make_check(
   preps = "namespace",
 
   gp = 'not import packages as a whole, as this can cause name
-        clashes between the imported packages. Instead, import
-        only the specific functions you need.',
+        clashes between the imported packages, especially over time as packages change.
+        Instead, import only the specific functions you need.',
 
   check = function(state) {
     if(inherits(state$namespace, "try-error")) return(NA)

diff --git a/R/my_linters.R b/R/my_linters.R
@@ -1,111 +1,3 @@
-
-#' @importFrom lintr Lint
-
-trailing_semicolon_linter <- function() lintr::Linter(function(source_file) {
-
-  allsc <- which(source_file$parsed_content$token == "';'")
-
-  if (!length(allsc)) return(list())
-
-  ## Check that it is the last token in the line
-  ## Note that we need to restrict the search to terminal
-  ## nodes, because parse is buggy and sometimes reports false,
-  ## too large end positions for non-terminal nodes.
-  badsc <- Filter(
-    x = allsc,
-    f = function(line) {
-      with(
-        source_file$parsed_content,
-        all(! terminal | line1 != line1[line] | col2 <= col2[line])
-      )
-    }
-  )
-
-  lapply(
-    badsc,
-    function(line) {
-      parsed <- source_file$parsed_content[line, ]
-      Lint(
-        filename = source_file$filename,
-        line_number = parsed$line1,
-        column_number = parsed$col1,
-        type = "style",
-        message = "Avoid trailing semicolons, they are not needed.",
-        line = source_file$lines[as.character(parsed$line1)],
-        ranges = list(c(parsed$col1, parsed$col2))
-      )
-    }
-  )
-})
-
-#' Find dangerous 1:x expressions
-#'
-#' Find occurrences of \code{1:length(x)}, \code{1:nrow(x)},
-#' \code{1:ncol(x)}, \code{1:NROW(x)}, \code{1:NCOL(x)} where
-#' \code{x} is an R expression.
-#'
-#' @return Lint object.
-#'
-#' @importFrom xmlparsedata xml_parse_data
-#' @importFrom xml2 read_xml xml_find_all xml_text xml_children xml_attr
-#' @keywords internal
-
-seq_linter <- function() lintr::Linter(function(source_file) {
-
-  if (!length(source_file$parsed_content)) return(list())
-
-  xml <- if ('xml_parsed_content' %in% names(source_file)) {
-    source_file$xml_parsed_content
-  } else {
-    read_xml(xml_parse_data(source_file$parsed_content))
-  }
-
-  bad_funcs <- c("length", "nrow", "ncol", "NROW", "NCOL")
-  text_clause <- paste0("text() = '", bad_funcs, "'", collapse = " or ")
-
-  xpath <- paste0(
-    "//expr",
-    "[expr[NUM_CONST[text()='1' or text()='1L']]]",
-    "[OP-COLON]",
-    "[expr[expr[SYMBOL_FUNCTION_CALL[", text_clause, "]]]]"
-  )
-
-  badx <- xml_find_all(xml, xpath)
-
-  ## The actual order of the nodes is document order
-  ## In practice we need to handle length(x):1
-  get_fun <- function(x, n) {
-    funcall <- xml_children(xml_children(x)[[n]])
-    if (!length(funcall)) return(NULL)
-    fun <- trim_ws(xml_text(funcall[[1]]))
-    if (! fun %in% bad_funcs) fun else paste0(fun, "(...)")
-  }
-
-  ## Unfortunately the more natural lapply(badx, ...) does not work,
-  ## because badx looses its class for length() and/or [[
-  lapply(
-    seq_along(badx),
-    function(i) {
-      x <- badx[[i]]
-      f1 <- get_fun(x, 1)
-      f2 <- get_fun(x, 3)
-      line1 <- xml_attr(x, "line1")
-      col1 <- xml_attr(x, "col1")
-      col2 <- xml_attr(x, "col1")
-      Lint(
-        filename = source_file$filename,
-        line_number = as.integer(line1),
-        column_number = as.integer(col1),
-        type = "warning",
-        message = paste0(
-          "Avoid ", f1, ":", f2, " expressions, use seq_len."),
-        line = source_file$lines[line1],
-        ranges = list(c(as.integer(col1), as.integer(col2)))
-      )
-    }
-  )
-})
-
 #' @importFrom lintr Lint
 
 dangerous_functions_linter <- function(source_file, funcs, type,

diff --git a/R/prep_lintr.R b/R/prep_lintr.R
@@ -5,12 +5,12 @@ linters_to_lint <- list(
   assignment_linter = lintr::assignment_linter(),
   line_length_linter = lintr::line_length_linter(80),
   package_hooks_linter = lintr::package_hooks_linter(),
-  trailing_semicolon_linter = trailing_semicolon_linter(),
+  semicolon_linter = lintr::semicolon_linter(allow_compound = TRUE),
   attach_detach_linter = attach_detach_linter(),
   setwd_linter = setwd_linter(),
   sapply_linter = sapply_linter(),
   library_require_linter = library_require_linter(),
-  seq_linter = seq_linter()
+  seq_linter = lintr::seq_linter()
 )
 
 #' @include lists.R

diff --git a/README.Rmd b/README.Rmd
@@ -78,4 +78,4 @@ results(g)[1:5,]
 
 ## License
 
-MIT © 2022 Ascent Digital Services UK Limited
+MIT © 2024 rOpenSci
diff --git a/README.md b/README.md
@@ -1,4 +1,3 @@
-
 # goodpractice <img src="man/figures/logo.png" align="right" width="20%" height="20%" />
 
 <!-- badges: start -->
@@ -28,7 +27,7 @@ install.packages("goodpractice")
 and the development version from GitHub
 
 ``` r
-remotes::install_github("ropensci-review-tools/goodpractice")
+pak::pak("ropensci-review-tools/goodpractice")
 ```
 
 ## Usage
@@ -48,59 +47,52 @@ g <- gp(pkg_path)
 ```
 
     #> ── R CMD build ────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
-    #>      checking for file ‘/tmp/RtmpjAXJO4/remotes2649f3077b5d9/badpackage/DESCRIPTION’ ...  ✔
-    #>   ─  i Preparing ‘badpackage’:
-    #>   ─  checking DESCRIPTION meta-information ...  ✔
-    #>   ─  checking vignette meta-information ...  ✔
-    #>   ─  checking for LF line-endings in source and make files and shell scripts (362ms)
+    #>      checking for file ‘/tmp/Rtmpoq5BBi/remotes9dcd65cdcc55/badpackage/DESCRIPTION’ ...  ✔  checking for file ‘/tmp/Rtmpoq5BBi/remotes9dcd65cdcc55/badpackage/DESCRIPTION’
+    #>   ─  preparing ‘badpackage’:
+    #>    checking DESCRIPTION meta-information ...  ✔  checking DESCRIPTION meta-information
+    #>      checking vignette meta-information ...  ✔  checking vignette meta-information
+    #>   ─  checking for LF line-endings in source and make files and shell scripts (400ms)
     #>   ─  checking for empty or unneeded directories
-    #>   ─  building ‘badpackage_1.0.0.tar.gz’
+    #> ─  building ‘badpackage_1.0.0.tar.gz’
+    #>      
+    #> 
 
 ``` r
 g
 ```
 
-    #> ── GP badpackage ───────────────────────────────────────────────────────────────
+    #> ── GP badpackage ──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
     #> 
     #> It is good practice to
     #> 
-    #>   ✖ not use "Depends" in DESCRIPTION, as it can cause name clashes, and
-    #>     poor interaction with other packages. Use "Imports" instead.
-    #>   ✖ omit "Date" in DESCRIPTION. It is not required and it gets invalid
-    #>     quite often. A build date will be added to the package when you
+    #>   ✖ not use "Depends" in DESCRIPTION, as it can cause name clashes, and poor interaction with other packages. Use "Imports" instead.
+    #>   ✖ omit "Date" in DESCRIPTION. It is not required and it gets invalid quite often. A build date will be added to the package when you
     #>     perform `R CMD build` on it.
-    #>   ✖ add a "URL" field to DESCRIPTION. It helps users find information
-    #>     about your package online. If your package does not have a
+    #>   ✖ add a "URL" field to DESCRIPTION. It helps users find information about your package online. If your package does not have a
     #>     homepage, add an URL to GitHub, or the CRAN package package page.
-    #>   ✖ add a "BugReports" field to DESCRIPTION, and point it to a bug
-    #>     tracker. Many online code hosting services provide bug trackers for
+    #>   ✖ add a "BugReports" field to DESCRIPTION, and point it to a bug tracker. Many online code hosting services provide bug trackers for
     #>     free, https://github.com, https://gitlab.com, etc.
-    #>   ✖ omit trailing semicolons from code lines. They are not needed and
-    #>     most R coding standards forbid them
+    #>   ✖ omit trailing semicolons from code lines. They are not needed and most R coding standards forbid them
     #> 
-    #>     R/semicolons.R:4:30
-    #>     R/semicolons.R:5:29
-    #>     R/semicolons.R:9:38
+    #>     'R/semicolons.R:4:30'
+    #>     'R/semicolons.R:5:29'
+    #>     'R/semicolons.R:9:38'
     #> 
-    #>   ✖ not import packages as a whole, as this can cause name clashes
-    #>     between the imported packages. Instead, import only the specific
+    #>   ✖ not import packages as a whole, as this can cause name clashes between the imported packages. Instead, import only the specific
     #>     functions you need.
-    #>   ✖ fix this R CMD check ERROR: VignetteBuilder package not declared:
-    #>     ‘knitr’ See section ‘The DESCRIPTION file’ in the ‘Writing R
+    #>   ✖ fix this R CMD check ERROR: VignetteBuilder package not declared: ‘knitr’ See section ‘The DESCRIPTION file’ in the ‘Writing R
     #>     Extensions’ manual.
-    #>   ✖ avoid 'T' and 'F', as they are just variables which are set to the
-    #>     logicals 'TRUE' and 'FALSE' by default, but are not reserved words
-    #>     and hence can be overwritten by the user.  Hence, one should always
-    #>     use 'TRUE' and 'FALSE' for the logicals.
+    #>   ✖ avoid 'T' and 'F', as they are just variables which are set to the logicals 'TRUE' and 'FALSE' by default, but are not reserved
+    #>     words and hence can be overwritten by the user.  Hence, one should always use 'TRUE' and 'FALSE' for the logicals.
     #> 
-    #>     R/tf.R:NA:NA
-    #>     R/tf.R:NA:NA
-    #>     R/tf.R:NA:NA
-    #>     R/tf.R:NA:NA
-    #>     R/tf.R:NA:NA
+    #>     'R/tf.R'
+    #>     'R/tf.R'
+    #>     'R/tf.R'
+    #>     'R/tf.R'
+    #>     'R/tf.R'
     #>     ... and 4 more lines
     #> 
-    #> ────────────────────────────────────────────────────────────────────────────────
+    #> ───────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
 
 ``` r
 # show all available checks
@@ -111,14 +103,13 @@ g_url <- gp(pkg_path, checks = "description_url")
 g_url
 ```
 
-    #> ── GP badpackage ───────────────────────────────────────────────────────────────
+    #> ── GP badpackage ──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
     #> 
     #> It is good practice to
     #> 
-    #>   ✖ add a "URL" field to DESCRIPTION. It helps users find information
-    #>     about your package online. If your package does not have a
+    #>   ✖ add a "URL" field to DESCRIPTION. It helps users find information about your package online. If your package does not have a
     #>     homepage, add an URL to GitHub, or the CRAN package package page.
-    #> ────────────────────────────────────────────────────────────────────────────────
+    #> ───────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
 
 ``` r
 # which checks were carried out?
@@ -136,7 +127,7 @@ failed_checks(g)
     #> [2] "no_description_date"                   
     #> [3] "description_url"                       
     #> [4] "description_bugreports"                
-    #> [5] "lintr_trailing_semicolon_linter"       
+    #> [5] "lintr_semicolon_linter"                
     #> [6] "no_import_package_as_a_whole"          
     #> [7] "rcmdcheck_package_dependencies_present"
     #> [8] "truefalse_not_tf"
@@ -155,4 +146,4 @@ results(g)[1:5,]
 
 ## License
 
-MIT © 2022 Ascent Digital Services UK Limited
+MIT © 2024 rOpenSci
diff --git a/man/seq_linter.Rd b/man/seq_linter.Rd
diff --git a/tests/testthat/test-lintr.R b/tests/testthat/test-lintr.R
@@ -1,5 +1,5 @@
 
-# "lintr_trailing_semicolon_linter" fails in bad1
+# "lintr_semicolon_linter" fails in bad1
 # "lintr_attach_detach_linter"  fails in bad2
 # "lintr_setwd_linter"  fails in bad2
 # "lintr_sapply_linter"  fails in bad2
@@ -10,7 +10,7 @@
 # "lintr_line_length_linter"
 
 gp_lintrs <- c("lintr_assignment_linter", "lintr_line_length_linter",
-               "lintr_trailing_semicolon_linter",
+               "lintr_semicolon_linter",
                "lintr_attach_detach_linter", "lintr_setwd_linter",
                "lintr_sapply_linter", "lintr_library_require_linter",
                "lintr_seq_linter")
@@ -41,10 +41,10 @@ test_that("lintr_line_length_linter", {
   # TODO expectation/example where the check fails
 
 })
-test_that("lintr_trailing_semicolon_linter", {
+test_that("lintr_semicolon_linter", {
 
-  expect_true(get_result(res_bad2, "lintr_trailing_semicolon_linter"))
-  expect_false(get_result(res_bad1, "lintr_trailing_semicolon_linter"))
+  expect_true(get_result(res_bad2, "lintr_semicolon_linter"))
+  expect_false(get_result(res_bad1, "lintr_semicolon_linter"))
 
 })
 test_that("lintr_attach_detach_linter", {
Original file line number	Diff line number	Diff line change
Expand Up		@@ -78,4 +78,4 @@ results(g)[1:5,]

		## License

		MIT © 2022 Ascent Digital Services UK Limited
		MIT © 2024 rOpenSci