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

MichaelChirico · 2023-12-19T06:31:40Z

Issues #2464 through #2493 are newly-formalized pending tasks spurred by this PR.

Some TODO were simply dropped (either I deemed them obsolete, or they were already fixed; I also dropped TODO in tests as not being the right place for them to live on as comments).

AshesITR · 2023-12-19T06:34:11Z

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


Issue reference lost

This is #2471. I took a bit of a judgment call in some cases on totally removing the TODO from the sources, here's one of them. I think it's a bit odd to anchor this TODO to the function overall, interrupting the roxygen2 mark-up, so I removed it.

codecov-commenter · 2023-12-19T06:34:51Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (11eae86) 98.55% compared to head (5241c83) 98.55%.

❗ Current head 5241c83 differs from pull request most recent head 8036e1b. Consider uploading reports for the commit 8036e1b to get more accurate results

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #2494   +/-   ##
=======================================
  Coverage   98.55%   98.55%           
=======================================
  Files         126      126           
  Lines        5762     5762           
=======================================
  Hits         5679     5679           
  Misses         83       83

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

AshesITR · 2023-12-19T06:35:01Z

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


Here I don't think the TODO anchors to this expression in particular, so I removed it. The issue is #2472.

R/literal_coercion_linter.R

AshesITR · 2023-12-19T06:36:45Z

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


I removed all of the TODO from the tests, I don't think they should live there.

For the record this is #2481.

AshesITR · 2023-12-19T06:46:17Z

To help with reviewing, please comment the issue number in the PR where you removed the comment. Thanks for the house keeping!

MichaelChirico · 2023-12-19T06:50:05Z

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")


these two are combined in #2470

MichaelChirico · 2023-12-19T06:50:58Z

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


MichaelChirico · 2023-12-19T06:51:25Z

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


This is done already, I think on the last release when we did pipe-awareness consistency.

MichaelChirico · 2023-12-19T06:51:39Z

tests/testthat/test-fixed_regex_linter.R

-#   > 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


MichaelChirico · 2023-12-19T06:51:52Z

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?


MichaelChirico · 2023-12-19T06:52:04Z

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?


MichaelChirico · 2023-12-19T06:52:29Z

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):


Both are #1043

MichaelChirico · 2023-12-19T06:52:47Z

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).


MichaelChirico · 2023-12-19T06:53:03Z

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


MichaelChirico · 2023-12-19T06:53:14Z

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


MichaelChirico · 2023-12-19T06:53:31Z

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


MichaelChirico · 2023-12-19T06:53:45Z

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


MichaelChirico · 2023-12-19T06:53:56Z

tests/testthat/test-unreachable_code_linter.R

-# })
-# nolint end: commented_code_linter.
-
-# TODO(michaelchirico): This could also apply to cases without


MichaelChirico · 2023-12-19T06:54:05Z

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,


Dropped this

MichaelChirico · 2023-12-19T06:54:19Z

tests/testthat/test-yoda_test_linter.R

-#   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?


MichaelChirico · 2023-12-19T06:54:39Z

tests/testthat/test-yoda_test_linter.R

-#   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


This is #1835

MichaelChirico · 2023-12-19T06:55:07Z

To help with reviewing, please comment the issue number in the PR where you removed the comment. Thanks for the house keeping!

SG, done.

AshesITR

Thanks for the cleanup and your patience with this review :)

MichaelChirico added 4 commits December 19, 2023 04:02

first batch of conversions

17cc495

finish R/

02c95a6

Fix regex

0e4d613

finish TODOs in tests

7eca989

AshesITR reviewed Dec 19, 2023

View reviewed changes

R/literal_coercion_linter.R Show resolved Hide resolved

AshesITR reviewed Dec 19, 2023

View reviewed changes

restore TODO

cf1dc38

MichaelChirico commented Dec 19, 2023

View reviewed changes

MichaelChirico added 2 commits December 21, 2023 15:42

Merge branch 'main' into todo-gh

571e1ca

Merge branch 'main' into todo-gh

a334dcf

bad merge

f691db5

AshesITR approved these changes Jan 8, 2024

View reviewed changes

Merge branch 'main' into todo-gh

8036e1b

MichaelChirico merged commit 15c556f into main Jan 9, 2024
19 of 20 checks passed

MichaelChirico deleted the todo-gh branch January 9, 2024 00:12

MichaelChirico mentioned this pull request Jan 9, 2024

Ensure roxygen2 available for GHA #2508

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

MichaelChirico commented Dec 19, 2023

AshesITR Dec 19, 2023

MichaelChirico Dec 19, 2023

codecov-commenter commented Dec 19, 2023 •

edited

Loading

AshesITR Dec 19, 2023

MichaelChirico Dec 19, 2023

AshesITR Dec 19, 2023

MichaelChirico Dec 19, 2023 •

edited

Loading

AshesITR commented Dec 19, 2023

MichaelChirico Dec 19, 2023

MichaelChirico Dec 19, 2023

MichaelChirico Dec 19, 2023

MichaelChirico Dec 19, 2023

MichaelChirico Dec 19, 2023

MichaelChirico Dec 19, 2023

MichaelChirico Dec 19, 2023

MichaelChirico Dec 19, 2023

MichaelChirico Dec 19, 2023

MichaelChirico Dec 19, 2023

MichaelChirico Dec 19, 2023

MichaelChirico Dec 19, 2023

MichaelChirico Dec 19, 2023

MichaelChirico Dec 19, 2023

MichaelChirico Dec 19, 2023

MichaelChirico Dec 19, 2023

MichaelChirico commented Dec 19, 2023

AshesITR left a comment

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

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

Conversation

MichaelChirico commented Dec 19, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Dec 19, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MichaelChirico Dec 19, 2023 • edited Loading

Choose a reason for hiding this comment

AshesITR commented Dec 19, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MichaelChirico commented Dec 19, 2023

AshesITR left a comment

Choose a reason for hiding this comment

codecov-commenter commented Dec 19, 2023 •

edited

Loading

MichaelChirico Dec 19, 2023 •

edited

Loading