Convert all TODO(user) to TODO(issue) (#2494)

* first batch of conversions * finish R/ * Fix regex * finish TODOs in tests * restore TODO * bad merge --------- Co-authored-by: AshesITR <[email protected]>
r-lib · Jan 9, 2024 · 15c556f · 15c556f
1 parent 11eae86
commit 15c556f
Show file tree

Hide file tree

Showing 27 changed files with 19 additions and 128 deletions.
diff --git a/.lintr b/.lintr
@@ -5,12 +5,9 @@ linters: all_linters(
     todo_comment_linter(
       except_regex = rex::rex(
         "TODO(",
-        group(or(
-          # GitHub issue number #1234, possibly from another repo org/repo#5678
-          list(maybe(one_or_more(alnum, "-"), "/", one_or_more(alnum, ".", "-", "_")), "#", one_or_more(digit)),
-          # GitHub user. TODO(#2450): remove this temporary immunity
-          one_or_more(alnum, "-")
-        )),
+        # GitHub issue number #1234, possibly from another repo org/repo#5678
+        maybe(one_or_more(character_class("a-zA-Z0-9-")), "/", one_or_more(character_class("a-zA-Z0-9._-"))),
+        "#", one_or_more(digit),
         ")"
       )
     ),

diff --git a/R/boolean_arithmetic_linter.R b/R/boolean_arithmetic_linter.R
@@ -63,7 +63,7 @@ boolean_arithmetic_linter <- function() {
     xml_nodes_to_lints(
       any_expr,
       source_expression = source_expression,
-      # TODO(michaelchirico): customize this?
+      # TODO(#2464): customize this?
       lint_message = paste(
         "Use any() to express logical aggregations.",
         "For example, replace length(which(x == y)) == 0 with !any(x == y)."

diff --git a/R/expect_length_linter.R b/R/expect_length_linter.R
@@ -21,7 +21,7 @@
 #' @seealso [linters] for a complete list of linters available in lintr.
 #' @export
 expect_length_linter <- function() {
-  # TODO(michaelchirico): also catch expect_true(length(x) == 1)
+  # TODO(#2465): also catch expect_true(length(x) == 1)
   xpath <- sprintf("
   parent::expr
     /following-sibling::expr[

diff --git a/R/indentation_linter.R b/R/indentation_linter.R
@@ -292,8 +292,7 @@ indentation_linter <- function(indent = 2L, hanging_indent_style = c("tidy", "al
         type = "style",
         message = lint_messages,
         line = unname(source_expression$file_lines[bad_lines]),
-        # TODO(AshesITR) when updating supported R version to R >= 4.1:
-        # replace by ranges = apply(lint_ranges, 1L, list, simplify = FALSE)
+        # TODO(#2467): Use ranges = apply(lint_ranges, 1L, list, simplify = FALSE).
         ranges = lapply(
           seq_along(bad_lines),
           function(i) {

diff --git a/R/inner_combine_linter.R b/R/inner_combine_linter.R
@@ -36,10 +36,7 @@ inner_combine_linter <- function() {
     "sqrt", "abs"
   )
 
-  # TODO(michaelchirico): the need to spell out specific arguments is pretty brittle,
-  #   but writing the xpath for the alternative case was proving too tricky.
-  #   It's messy enough as is -- it may make sense to take another pass at
-  #   writing the xpath from scratch to see if it can't be simplified.
+  # TODO(#2468): Try and make this XPath less brittle/more extensible.
 
   # See ?as.Date, ?as.POSIXct. tryFormats is not explicitly in any default
   #   POSIXct method, but it is in as.Date.character and as.POSIXlt.character --

diff --git a/R/is_numeric_linter.R b/R/is_numeric_linter.R
@@ -34,11 +34,9 @@
 #' @seealso [linters] for a complete list of linters available in lintr.
 #' @export
 is_numeric_linter <- function() {
-  # TODO(michaelchirico): this should also cover is.double(x) || is.integer(x)
-  # TODO(#1636): is.numeric(x) || is.integer(x) || is.factor(x) is also redundant
-  # TODO(michaelchirico): consdier capturing any(class(x) == "numeric/integer")
-  #   here directly; currently we rely on class_equals_linter() also active
-  # TODO(michaelchirico): also catch inherits(x, c("numeric", "integer"))
+  # TODO(#2469): This should also cover is.double(x) || is.integer(x).
+  # TODO(#1636): is.numeric(x) || is.integer(x) || is.factor(x) is also redundant.
+  # TODO(#2470): Consider usages with class(), typeof(), or inherits().
   is_numeric_expr <- "expr[1][SYMBOL_FUNCTION_CALL[text() = 'is.numeric']]"
   is_integer_expr <- "expr[1][SYMBOL_FUNCTION_CALL[text() = 'is.integer']]"
 
@@ -55,7 +53,6 @@ is_numeric_linter <- function() {
   ")
 
   # testing class(x) %in% c("numeric", "integer")
-  # TODO(michaelchirico): include typeof(x) %in% c("integer", "double")
   class_xpath <- "
   //SPECIAL[
     text() = '%in%'

diff --git a/R/keyword_quote_linter.R b/R/keyword_quote_linter.R
@@ -46,11 +46,6 @@
 #' @evalRd rd_tags("keyword_quote_linter")
 #' @seealso [linters] for a complete list of linters available in lintr.
 #' @export
-# TODO(michaelchirico): offer a stricter version of this that
-#   requires backticks to be used for non-syntactic names (i.e., not quotes).
-#   Here are the relevant xpaths:
-#   //expr[expr[SYMBOL_FUNCTION_CALL]]/SYMBOL_SUB[starts-with(text(), '`')]
-#   //expr[expr[SYMBOL_FUNCTION_CALL]]/STR_CONST[{is_quoted(text())}]
 keyword_quote_linter <- function() {
   # Check if a string could be assigned as an R variable.
   #

diff --git a/R/list_comparison_linter.R b/R/list_comparison_linter.R
@@ -21,8 +21,6 @@
 #' @seealso [linters] for a complete list of linters available in lintr.
 #' @export
 list_comparison_linter <- function() {
-  # TODO(michaelchirico): extend to cases where using simplify=FALSE implies a
-  #   list output, e.g. with sapply, replicate, mapply.
   list_mapper_alternatives <- c(
     lapply = "vapply(x, FUN, character(1L))",
     map = "map_chr(x, FUN)",

diff --git a/R/literal_coercion_linter.R b/R/literal_coercion_linter.R
@@ -98,9 +98,7 @@ literal_coercion_linter <- function() {
         coercion_str[needs_prefix] <- paste0("rlang::", coercion_str[needs_prefix])
       }
       # the linter logic & rlang requirement should ensure that it's safe to run eval() here
-      # TODO(michaelchirico): this recommends '1' to replace as.numeric(1), where our
-      #   own implicit_integer_linter(), if active, would require this to be 1.0. Should
-      #   we recommend this instead, or offer it as an alternative?
+      # TODO(#2473): Avoid a recommendation like '1' that clashes with implicit_integer_linter().
       literal_equivalent_str <- vapply(str2expression(coercion_str), function(expr) deparse1(eval(expr)), character(1L))
       lint_message <- sprintf(
         "Use %s instead of %s, i.e., use literals directly where possible, instead of coercion.",

diff --git a/R/object_usage_linter.R b/R/object_usage_linter.R
@@ -209,7 +209,7 @@ parse_check_usage <- function(expression,
   # nocov start
   is_missing <- is.na(res$message)
   if (any(is_missing)) {
-    # TODO(AshesITR): Remove this in the future, if no bugs arise from this safeguard
+    # TODO(#2474): Remove this.
     warning(
       "Possible bug in lintr: Couldn't parse usage message ", sQuote(vals[is_missing][[1L]]), ". ",
       "Ignoring ", sum(is_missing), " usage warnings. Please report an issue at https://github.com/r-lib/lintr/issues.",

diff --git a/R/shared_constants.R b/R/shared_constants.R
@@ -256,7 +256,7 @@ extract_glued_symbols <- function(expr, interpret_glue) {
 
   glued_symbols <- new.env(parent = emptyenv())
   for (glue_call in glue_calls) {
-    # TODO(michaelchirico): consider dropping tryCatch() here if we're more confident in our logic
+    # TODO(#2475): Drop tryCatch().
     parsed_call <-
       tryCatch(xml2lang(glue_call), error = unexpected_glue_parse_error, warning = unexpected_glue_parse_error)
     parsed_call[[".envir"]] <- glued_symbols

diff --git a/R/unnecessary_concatenation_linter.R b/R/unnecessary_concatenation_linter.R
@@ -104,7 +104,7 @@ unnecessary_concatenation_linter <- function(allow_single_expression = TRUE) { #
       as.integer(!is.na(xml_find_first(c_calls, to_pipe_xpath)))
     # NB: the xpath guarantees num_args is 0, 1, or 2. 2 comes
     #   in "a" %>% c("b").
-    # TODO(michaelchirico): can we handle this all inside the XPath with reasonable concision?
+    # TODO(#2476): Push this logic back into the XPath.
     is_unneeded <- num_args <= 1L
     c_calls <- c_calls[is_unneeded]
     num_args <- num_args[is_unneeded]

diff --git a/R/unnecessary_lambda_linter.R b/R/unnecessary_lambda_linter.R
@@ -112,7 +112,7 @@ unnecessary_lambda_linter <- function(allow_comparison = FALSE) {
   # outline:
   #   1. match one of the identified mappers
   #   2. match an anonymous function that can be "symbol-ized"
-  #     a. it's a one-variable function [TODO(michaelchirico): is this necessary?]
+  #     a. it's a one-variable function [TODO(#2477): relax this]
   #     b. the function is a single call
   #     c. that call's _first_ argument is just the function argument (a SYMBOL)
   #       - and it has to be passed positionally (not as a keyword)
@@ -160,11 +160,7 @@ unnecessary_lambda_linter <- function(allow_comparison = FALSE) {
     default_calls <- source_expression$xml_find_function_calls(apply_funs)
     default_fun_expr <- xml_find_all(default_calls, default_fun_xpath)
 
-    # TODO(michaelchirico): further message customization is possible here,
-    #   e.g. don't always refer to 'lapply()' in the example, and customize to
-    #   whether arguments need to be subsumed in '...' or not. The trouble is in
-    #   keeping track of which argument the anonymous function is supplied (2nd
-    #   argument for many calls, but 3rd e.g. for apply())
+    # TODO(#2478): Give a specific recommendation in the message.
     default_call_fun <- xml_text(xml_find_first(default_fun_expr, fun_xpath))
     default_symbol <- xml_text(xml_find_first(default_fun_expr, symbol_xpath))
     default_fun_lints <- xml_nodes_to_lints(

diff --git a/R/unused_import_linter.R b/R/unused_import_linter.R
@@ -109,8 +109,7 @@ unused_import_linter <- function(allow_ns_usage = FALSE,
       logical(1L)
     )
 
-    # TODO(michaelchirico): instead of vectorizing over packages,
-    #   xml_find_all SYMBOL_PACKAGE namespaces and check imported_pkgs %in%
+    # TODO(#2480): Only call //SYMBOL_PACKAGE once.
     is_ns_used <- vapply(
       imported_pkgs,
       function(pkg) {

diff --git a/tests/testthat/test-any_duplicated_linter.R b/tests/testthat/test-any_duplicated_linter.R
@@ -49,10 +49,6 @@ test_that("any_duplicated_linter catches length(unique()) equivalencies too", {
   expect_lint("length(unique(x)) != length(x)", lint_msg_x, linter)
   expect_lint("length(unique(x)) < length(x)", lint_msg_x, linter)
   expect_lint("length(x) > length(unique(x))", lint_msg_x, linter)
-
-  # TODO(michaelchirico): try and match data.table- and dplyr-specific versions of
-  #   this, e.g. DT[, length(unique(col)) == .N] or
-  #   > DT %>% filter(length(unique(col)) == n())
 })
 
 test_that("any_duplicated_linter catches expression with two types of lint", {

diff --git a/tests/testthat/test-expect_s3_class_linter.R b/tests/testthat/test-expect_s3_class_linter.R
@@ -60,14 +60,6 @@ test_that("expect_s3_class_linter blocks simple disallowed usages", {
     rex::rex("expect_s3_class(x, k) is better than expect_true(is.<k>(x))"),
     linter
   )
-
-  # TODO(michaelchirico): consider more carefully which sorts of class(x) %in% . and
-  #   . %in% class(x) calls should be linted
-  #> expect_lint(
-  #>   "expect_true('lm' %in% class(x))",
-  #>   "expect_s3_class\\(x, k\\) is better than expect_equal\\(class\\(x\\), k",
-  #>   expect_s3_class_linter
-  #> )
 })
 
 local({

diff --git a/tests/testthat/test-fixed_regex_linter.R b/tests/testthat/test-fixed_regex_linter.R
@@ -270,19 +270,6 @@ test_that("fixed replacement is correct with UTF-8", {
   )
 })
 
-# TODO(michaelchirico): one difference for stringr functions vs. base is that
-#   stringr is much friendlier to piping, so that
-#   > str %>% str_replace_all("x$", "y")
-#   actually doesn't need fixed(), but the logic now is only looking at "y"
-#   since it's the second argument and a non-regex string. Similarly,
-#   > str %>% str_detect("x")
-#   is a false negative. thankfully there appear to be few false positives here
-
-# TODO(michaelchirico): we could in principle build in logic to detect whether
-#   perl=TRUE and interpret "regex or not" accordingly. One place
-#   up in practice is for '\<', which is a special character in default
-#   regex but not in PCRE. Empirically relevant for HTML-related regex e.g. \\<li\\>
-
 #' Generate a string with a non-printable Unicode entry robust to test environment
 #'
 #' Non-printable unicode behaves wildly different with `encodeString()`

diff --git a/tests/testthat/test-if_not_else_linter.R b/tests/testthat/test-if_not_else_linter.R
@@ -83,5 +83,3 @@ test_that("exceptions= argument works", {
 
   expect_lint("if (!foo(x)) y else z", NULL, if_not_else_linter(exceptions = "foo"))
 })
-
-# TODO(michaelchirico): should if (A != B) be considered as well?
diff --git a/tests/testthat/test-ifelse_censor_linter.R b/tests/testthat/test-ifelse_censor_linter.R
@@ -81,6 +81,3 @@ test_that("lints vectorize", {
     ifelse_censor_linter()
   )
 })
-
-# TODO(michaelchirico): how easy would it be to strip parens when considering lint?
-#   e.g. ifelse(x < (kMaxIndex - 1), x, kMaxIndex - 1)
diff --git a/tests/testthat/test-inner_combine_linter.R b/tests/testthat/test-inner_combine_linter.R
@@ -89,7 +89,7 @@ patrick::with_parameters_test_that(
     "present/absent arg (POSIXct)",   "c(as.POSIXct(x, format = '%y'), as.POSIXct(y))",
     "mismatched arg (log)",           "c(log(x, base = 4), log(y, base = 5))",
     "present/absent arg (log)",       "c(log(x, base = 4), log(y))"
-    # TODO(michaelchirico): fix the code so these edge cases are covered
+    # TODO(#2486): Reactivate these.
     # "unknown Date method argument",    "c(as.Date(x, zoo = zzz), as.Date(y, zoo = zzz))",
     # "known+unknown Date argument", "c(as.Date(x, format = '%y', zoo = zzz), as.Date(y, format = '%y', zoo = zzz))",
     # "unknown POSIXct method argument", "c(as.POSIXct(x, zoo = zzz), as.POSIXct(y, zoo = zzz))",

diff --git a/tests/testthat/test-knitr_formats.R b/tests/testthat/test-knitr_formats.R
@@ -94,18 +94,12 @@ test_that("it handles tex", {
     file = test_path("knitr_formats", "test.Rtex"),
     checks = list(
       list(regexes[["indent"]], line_number = 11L),
-      # TODO(AshesITR):
-      # masking the Rtex escape char by whitespace causes false-positive indentation lints
       list(regexes[["assign"]], line_number = 11L),
       list(regexes[["indent"]], line_number = 22L),
       list(regexes[["local_var"]], line_number = 23L),
       list(regexes[["assign"]], line_number = 23L),
       list(regexes[["trailing"]], line_number = 25L),
       list(regexes[["trailws"]], line_number = 25L)
-      # TODO(AshesITR): #1043
-      # file_lines contains a whitespace on the final line for Rtex, because that is used to mark the Rtex escape char
-      # "%" as well.
-      # cf. get_source_expressions("tests/testthat/knitr_formats/test.Rtex")$lines[[25]]
     ),
     linters = default_linters,
     parse_settings = FALSE

diff --git a/tests/testthat/test-nzchar_linter.R b/tests/testthat/test-nzchar_linter.R
@@ -17,9 +17,6 @@ test_that("nzchar_linter skips as appropriate for other nchar args", {
   # type="bytes" should be >= the value for the default (type="chars")
   expect_lint("nchar(x, type='width') == 0L", NULL, linter)
 
-  # TODO(michaelchirico): check compatibility of nchar(., allowNA=TRUE).
-  #   there are no examples in ?nchar, nor any relevant usages on StackOverflow.
-  #   just assume they are incompatible now to be conservative.
   expect_lint("nchar(x, allowNA=TRUE) == 0L", NULL, linter)
 
   # nzchar also has keepNA argument so a drop-in switch is easy

diff --git a/tests/testthat/test-one_call_pipe_linter.R b/tests/testthat/test-one_call_pipe_linter.R
@@ -21,9 +21,7 @@ test_that("one_call_pipe_linter blocks simple disallowed usages", {
   # new lines don't matter
   expect_lint("x %>%\n  foo()", lint_msg, linter)
 
-  # catch the "inner" pipe chain, not the "outer" one
-  # TODO(michaelchirico): actually, this should lint twice -- we're too aggressive
-  #   in counting _all_ nested calls.
+  # nested case
   expect_lint("x %>% inner_join(y %>% filter(is_treatment))", lint_msg, linter)
 })
 

diff --git a/tests/testthat/test-unnecessary_lambda_linter.R b/tests/testthat/test-unnecessary_lambda_linter.R
@@ -167,8 +167,6 @@ test_that("unnecessary_lambda_linter doesn't apply to keyword args", {
 test_that("purrr-style anonymous functions are also caught", {
   linter <- unnecessary_lambda_linter()
 
-  # TODO(michaelchirico): this is just purrr::flatten(x). We should write another
-  #   linter to encourage that usage.
   expect_lint("purrr::map(x, ~.x)", NULL, linter)
   expect_lint("purrr::map_df(x, ~lm(y, .x))", NULL, linter)
   expect_lint("map_dbl(x, ~foo(bar = .x))", NULL, linter)

diff --git a/tests/testthat/test-unnecessary_nesting_linter.R b/tests/testthat/test-unnecessary_nesting_linter.R
@@ -27,9 +27,6 @@ test_that("unnecessary_nesting_linter skips allowed usages", {
   )
 })
 
-# TODO(michaelchirico): consider if there's a nice easy pattern to enforce for
-#   multiple if/else cases. This test in particular would be easy to un-nest,
-#   but it's not true in general.
 test_that("Multiple if/else statements don't require unnesting", {
   # with further branches, reducing nesting might be less readable
   expect_lint(

diff --git a/tests/testthat/test-unreachable_code_linter.R b/tests/testthat/test-unreachable_code_linter.R
@@ -720,30 +720,3 @@ test_that("allow_comment_regex= obeys covr's custom exclusion when set", {
     linter_covr
   )
 })
-
-# nolint start: commented_code_linter.
-# TODO(michaelchirico): extend to work on switch() statements
-# test_that("unreachable_code_linter interacts with switch() as expected", {
-#   unreachable_inside_switch_lines <- trim_some("
-#     foo <- function(x) {
-#       switch(x,
-#         a = {
-#           return(x)
-#           x + 1
-#         },
-#         b = {
-#           return(x + 1)
-#         }
-#       )
-#     }
-#   ")
-#   expect_lint(
-#     unreachable_inside_switch_lines,
-#     rex::rex("Code and comments coming after a return() or stop()"),
-#     unreachable_code_linter()
-#   )
-# })
-# nolint end: commented_code_linter.
-
-# TODO(michaelchirico): This could also apply to cases without
-#   explicit returns (where it can only apply to comments)
diff --git a/tests/testthat/test-yoda_test_linter.R b/tests/testthat/test-yoda_test_linter.R
@@ -61,15 +61,3 @@ test_that("lints vectorize", {
     yoda_test_linter()
   )
 })
-
-# TODO(michaelchirico): Should this be extended to RUnit tests? It seems yes,
-#   but the argument names in RUnit (inherited from base all.equal()) are a bit
-#   confusing, e.g. `checkEqual(target=, current=)`. From the name, one might
-#   reasonably conclude 'expected' comes first, and 'actual' comes second.
-# TODO(michaelchirico): What sorts of combinations of literals can be included?
-#   e.g. expect_equal(c(1, 2), x) is a yoda test; is expect_equal(c(x, 1), y)?
-#   clearly it's not true for general f() besides c(). What about other
-#   constructors of literals? data.frame(), data.table(), tibble(), ...?
-# TODO(michaelchirico): The logic could also be extended to "tests" inside regular
-#   code, not just test suites, e.g. `if (2 == x)`, `while(3 <= x)`,
-#   `stopifnot('a' == foo(y))`.
Original file line number	Diff line number	Diff line change
Expand Up		@@ -83,5 +83,3 @@ test_that("exceptions= argument works", {

		expect_lint("if (!foo(x)) y else z", NULL, if_not_else_linter(exceptions = "foo"))
		})

		# TODO(michaelchirico): should if (A != B) be considered as well?