seq_linter lints on dplyr and data.table flavors
#1398
Conversation
R/seq_linter.R
Outdated
| and ( | ||
| expr[expr[(expr|self::*)[SYMBOL_FUNCTION_CALL[ {xp_text_in_table(bad_funcs)} ]]]] | ||
| or | ||
| expr[SYMBOL='.N'] |
There was a problem hiding this comment.
oh, I didn't know this works!
usually we'd do
SYMBOL[text() = '.N']
let me check the docs to ensure there's nothing subtle about this XPath usage
There was a problem hiding this comment.
nice! looks like this is legit:
If one object to be compared is a node-set and the other is a string, then the comparison will be true if and only if there is a node in the node-set such that the result of performing the comparison on the string-value of the node and the other string is true.
https://www.w3.org/TR/1999/REC-xpath-19991116/#section-Expressions
Its for != where things are a bit trickier:
tests/testthat/test-seq_linter.R
Outdated
| ) | ||
|
|
||
| # `.N` is a symbol, so the message should not print it as a function | ||
| expect_error(expect_lint( |
There was a problem hiding this comment.
I would just improve the regex in the first test rather than add a second test
tests/testthat/test-seq_linter.R
Outdated
| ) | ||
|
|
||
| # `dplyr::n()` takes no arguments, so the message shouldn't have `n(...)` | ||
| expect_error(expect_lint( |
There was a problem hiding this comment.
not necessary, already implied by the first test
There was a problem hiding this comment.
(btw I think testthat offers expect_failure() for expecting failed expectations, and expect_success for the reverse)
|
@MichaelChirico Thanks! I've improved the regex and removed the redundant tests. |
|
Thanks for your work! 🙌 |
Closes #1396