Support case where specified method ignores mode in download_linter()

R/download_file_linter.R

@@ -3,6 +3,7 @@
 #' `mode = "w"` (the default) or `mode = "a"` in `download.file()` can generate broken files
 #' on Windows. Instead, [utils::download.file()] recommends the usage of `mode = "wb"`
 #' and `mode = "ab"`.
+#' If `method = "curl"` or `method = "wget"`, no `mode` should be provided as it will be ignored.
 #'
 #' @examples
 #' # will produce lints
@@ -16,12 +17,22 @@
 #'   linters = download_file_linter()
 #' )
 #'
+#' lint(
+#'   text = "download.file(x = my_url, method = 'curl', mode = 'wb')",
+#'   linters = download_file_linter()
+#' )
+#'
 #' # okay
 #' lint(
 #'   text = "download.file(x = my_url, mode = 'wb')",
 #'   linters = download_file_linter()
 #' )
 #'
+#' lint(
+#'   text = "download.file(x = my_url, method = 'curl')",
+#'   linters = download_file_linter()
+#' )
+#'
 #' @evalRd rd_tags("download_file_linter")
 #' @seealso [linters] for a complete list of linters available in lintr.
 #' @export
@@ -38,16 +49,38 @@ download_file_linter <- function() {
       "
     )
 
-    implicit_mode <- is.na(download_file_mode)
+    has_implicit_mode <- is.na(download_file_mode)
+
+    # According to download.file() documentation, mode is ignored when method
+    # is "curl" or "wget" so if it's explicitly specified, we should let the
+    # user know they might not get the desired effect
+    download_file_method <- get_r_string(
+      download_file_calls,
+      "parent::expr[1]/
+        SYMBOL_SUB[text() = 'method']/
+        following-sibling::expr[1]/STR_CONST
+      "
+    )
+    has_ignored_mode <- !is.na(download_file_method) & download_file_method %in% c("wget", "curl")
+
+    ignored_mode_lint <- xml_nodes_to_lints(
+      download_file_calls[!has_implicit_mode & has_ignored_mode],
+      source_expression = source_expression,
+      lint_message = sprintf(
+        "mode argument value is ignored for download.file(method = '%s').",
+        download_file_method[has_ignored_mode]
+      ),
+      type = "warning"
+    )
 
     implicit_mode_lint <- xml_nodes_to_lints(
-      download_file_calls[implicit_mode],
+      download_file_calls[has_implicit_mode & !has_ignored_mode],
       source_expression = source_expression,
       lint_message = "download.file() should use mode = 'wb' or ('ab') rather than relying on the default mode = 'w'.",
       type = "warning"
     )
 
-    wrong_mode <- !implicit_mode & download_file_mode %in% c("a", "w")
+    wrong_mode <- !has_implicit_mode & !has_ignored_mode & download_file_mode %in% c("a", "w")
 
     explicit_wrong_mode_lint <- xml_nodes_to_lints(
       download_file_calls[wrong_mode],
@@ -59,7 +92,7 @@ download_file_linter <- function() {
       type = "warning"
     )
 
-    c(implicit_mode_lint, explicit_wrong_mode_lint)
+    c(implicit_mode_lint, ignored_mode_lint, explicit_wrong_mode_lint)
   })
 
 }

tests/testthat/test-download_file_linter.R

@@ -7,6 +7,13 @@ test_that("download_file_linter skips allowed usages", {
   # 'w' or 'a' but passed to different arguments
   expect_no_lint("download.file(x, destfile = 'w', mode = 'wb')", linter)
   expect_no_lint("download.file(x, mode = 'wb', method = 'internal', quiet = TRUE, 'w')", linter)
+
+  # if mode = "wget" or "curl", mode is ignored so we don't lint on the default
+  expect_no_lint("download.file(x, method = 'curl')", linter)
+  expect_no_lint("download.file(x, method = 'wget')", linter)
+
+  expect_no_lint("download.file(x, method = 'internal', mode = 'wb')", linter)
+  expect_no_lint("download.file(x, method = method, mode = 'wb')", linter)
 })
 
 test_that("download_file_linter blocks simple disallowed usages", {
@@ -15,21 +22,29 @@ test_that("download_file_linter blocks simple disallowed usages", {
 
   # Case 1: implicit default (mode = "w")
   expect_lint("download.file(x)", lint_message, linter)
+  expect_lint("download.file(x, method = 'internal')", lint_message, linter)
 
   # Case 2: non-portable mode specified by name
   expect_lint("download.file(x, mode = 'w')", lint_message, linter)
   expect_lint("download.file(x, mode = 'a')", lint_message, linter)
+  expect_lint("download.file(x, method = method, mode = 'w')", lint_message, linter)
 
   # 'wb' passed to different argument
   expect_lint("download.file(x, mode = 'w', method = 'internal', quiet = TRUE, 'wb')", lint_message, linter)
   expect_lint("download.file(cacheOK = TRUE, destfile, method, quiet, x = 'wb')", lint_message, linter)
+
+  # mode explicitly specified with a method that ignores it
+  lint_message <- rex::rex("mode argument value is ignored")
+  expect_lint("download.file(x, method = 'wget', mode = 'w')", lint_message, linter)
+  expect_lint("download.file(x, method = 'curl', mode = 'wb')", lint_message, linter)
 })
 
 test_that("lints vectorize", {
   expect_lint(
     trim_some("{
       download.file(x, mode = 'w')
       download.file(y, mode = 'a')
+      download.file(z, mode = 'wb', method = 'curl')
     }"),
     list(
       list(
@@ -39,6 +54,10 @@ test_that("lints vectorize", {
       list(
         rex::rex("download.file() should use mode = 'ab'"),
         line_number = 3L
+      ),
+      list(
+        rex::rex("mode argument value is ignored"),
+        line_number = 4L
       )
     ),
     download_file_linter()