-
Notifications
You must be signed in to change notification settings - Fork 186
Add grepv() awareness to the package #2856
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2856 +/- ##
=======================================
Coverage 99.64% 99.64%
=======================================
Files 127 127
Lines 7068 7070 +2
=======================================
+ Hits 7043 7045 +2
Misses 25 25 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Potentially in a follow up PR, but if we start recommending grepv()
(which I agree with), it would be good to also lint grep(, value = TRUE)
.
@@ -72,7 +72,7 @@ regex_subset_linter <- function() { | |||
grep_expr, | |||
source_expression = source_expression, | |||
lint_message = | |||
"Prefer grep(pattern, x, ..., value = TRUE) over x[grep(pattern, x, ...)] and x[grepl(pattern, x, ...)].", | |||
"Prefer grepv(pattern, x, ...) over x[grep(pattern, x, ...)] and x[grepl(pattern, x, ...)].", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we maybe customize this error message based on the R version since lintr is still compatible with R 4.0 to 4.4, which don't know about grepv()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wouldn't go that way, since the process running {lintr} can be different from the DESCRIPTION dependency, or any other number of incompatibilities between the report and the actual intention.
WDYT about just adding
Use {backports} if your code might also run on R versions before 4.5.0.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good in theory but we would have to wait until r-lib/backports#88 is merged and a new version of backports released.
Maybe the safest option is to be more verbose:
Prefer grepv(pattern, x, ...), or grep(pattern, x, value = TRUE) if your code might also run on R versions before 4.5.0, over x[grep(pattern, x, ...)] and x[grepl(pattern, x, ...)].
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adjusted, PTAL.
Yea, I've been meaning to file that as an issue for a new linter (presumably |
Also fixes #2648 I think. |
Closes #2855. Closes #2648. Progress on #2737