Lint nrow, ncol, NROW and NCOL with logical expressions #2952
Conversation
R/length_test_linter.R
Outdated
| @@ -1,4 +1,4 @@ | |||
| #' Check for a common mistake where length is applied in the wrong place | |||
| #' Check for a common mistake where length/nrow/ncol/NROW/NCOL is applied in the wrong place | |||
There was a problem hiding this comment.
| #' Check for a common mistake where length/nrow/ncol/NROW/NCOL is applied in the wrong place | |
| #' Check for a common mistake where a size check like 'length' is applied in the wrong place |
There was a problem hiding this comment.
And mention nrow/ncol/NROW/NCOL in the description below, as well as adding one new example for any of those.
R/length_test_linter.R
Outdated
| lint_message <- sprintf( | ||
| "Checking the length of a logical vector is likely a mistake. Did you mean `length(%s) %s %s`?", | ||
| expr_parts[1L, ], expr_parts[2L, ], expr_parts[3L, ] | ||
| "Checking the length of a logical vector is likely a mistake. Did you mean `%s(%s) %s %s`?", |
There was a problem hiding this comment.
Maybe it should be "Checking the %s of a logical vector" too?
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2952 +/- ##
=======================================
Coverage 99.24% 99.24%
=======================================
Files 129 129
Lines 7276 7277 +1
=======================================
+ Hits 7221 7222 +1
Misses 55 55 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| ) | ||
| }, | ||
| fun = funs, | ||
| .test_name = names(fun) |
There was a problem hiding this comment.
I think we can drop .test_name here and just use fun = c("length", ...)
| ops <- c(lt = "<", lte = "<=", gt = ">", gte = ">=", eq = "==", neq = "!=") | ||
| funs <- rep(c(length = "length", nrow = "nrow", ncol = "ncol", NROW = "NROW", NCOL = "NCOL"), | ||
| each = length(ops)) | ||
| ops <- rep(ops, length(unique(funs))) |
There was a problem hiding this comment.
this looks like expand.grid(), right? You can use .cases= in with_parameters_test_that() here, c.f.
lintr/tests/testthat/test-list_comparison_linter.R
Lines 9 to 18 in aff43f1
* Lint nrow, ncol, NROW and NCOL with logical expressions. * Address review comments. * Fix indentation. * tidy up tests --------- Co-authored-by: Michael Chirico <chiricom@google.com>
2 participants
I've taken a stab to fix #2933. I'm still unsure about the wording in the help page, at the moment I've only applied minimal changes. Also in terms of tests, I've used
patrickin one case, but manually added some other no_lint tests, as I was not sure that it was worth the effort. I'm open to suggestions!