-
Notifications
You must be signed in to change notification settings - Fork 187
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add potential sequence()
lints to seq_linter()
#2618
base: main
Are you sure you want to change the base?
Conversation
NEWS.md
Outdated
@@ -32,6 +32,7 @@ | |||
|
|||
## New and improved features | |||
|
|||
* `seq_linter()` now includes lints to inform about missed opportunities to use the `sequence()` base R function (#2618, @Bisaloo) |
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 think sequence()
is not a very well-known function (<500 calls on CRAN vs more than 80K for seq()
), here is a good opportunity to quickly introduce it:
"to use the sequence()
function, e.g. unlist(lapply(ints, seq))
"
R/seq_linter.R
Outdated
sequence_xpath <- glue(" | ||
//SYMBOL_FUNCTION_CALL[ { map_funcs } ] | ||
/parent::expr/parent::expr[ | ||
preceding-sibling::expr/SYMBOL_FUNCTION_CALL[text() = 'unlist'] |
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.
nit: I would try and keep the logic in one direction on the AST: check sapply, then check seq_len, then check unlist, roughly
//SYMBOL_FUNCTION_CALL/parent::expr[following-sibling::expr/SYMBOL]/parent::expr/parent::expr[expr/SYMBOL_FUNCTION_CALL]
Also, please switch to use the new find_function_calls
approach, which is appropriate here, e.g.
lintr/R/condition_call_linter.R
Line 83 in 6ed78c2
xml_calls <- source_expression$xml_find_function_calls(c("stop", "warning")) |
lint_msg <- rex::rex("Use sequence()") | ||
|
||
expect_lint( | ||
"unlist(lapply(x, seq_len))", |
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.
The lint now only covers unlist(lapply(x, seq_len))
.
What about
unlist(lapply(x, seq))
unlist(lapply(x, seq, by = 2))
, and other variants equivalent to using thefrom=
and/orby=
arguments ofsequence()
?- I don't think it's worth trying to find examples like
lapply(x, \(xi) seq(xi))
which would be covered byunnecessary_lambda_linter()
. But are there any examples that do require looking into a lambda to replicate asequence()
-able call?
(it would be nice to get a sense of how many hits these are getting before investing too much time -- and also feel free to earmark some of this as a TODO in a follow-up issue, though lapply(x,seq)
is trivial enough to be done here)
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.
unlist(lapply(x, seq, by = 2))
, and other variants equivalent to using thefrom=
and/orby=
arguments of sequence()?
I don't think there is a direct equivalence:
-
from
has a different behaviour inseq()
andsequence()
unlist(lapply(1:5, seq, from = 2)) #> [1] 2 1 2 2 3 2 3 4 2 3 4 5 sequence(1:5, from = 2) #> [1] 2 2 3 2 3 4 2 3 4 5 2 3 4 5 6
Created on 2024-06-22 with reprex v2.1.0
-
As far as I can tell,
by=
cannot be used on its own withoutfrom=
, which brings us back to the previous caseseq(20, by = 2) #> Error in seq.default(20, by = 2): wrong sign in 'by' argument
Created on 2024-06-22 with reprex v2.1.0
I have edited the xpath to make sure this type of call doesn't lint.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2618 +/- ##
==========================================
- Coverage 98.15% 97.97% -0.18%
==========================================
Files 125 126 +1
Lines 5738 5774 +36
==========================================
+ Hits 5632 5657 +25
- Misses 106 117 +11 ☔ View full report in Codecov by Sentry. |
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.
Let's also wait for MC's approval.
@MichaelChirico Can we add @Bisaloo to the repo as a contributor? Of course, only if he wishes to. 😅
Fix #2595