-
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
New yoda_test_linter #957
New yoda_test_linter #957
Conversation
# TODO(michaelchirico): What sorts of combinations of literals can be included? | ||
# 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(), ...? |
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.
Maybe it's worth to craft up an "extended literal" XPath that contains a few whitelisted functions and use that throughout lintr if we want to match a "constant" expression?
First thought would be something like not(SYMBOL_FUNCTION_CALL[not(text() = 'c' or ...)])
etc.
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 great, I was thinking something similar. We have something like that in StringsAsFactorsLinter
as well (which also has some logic about mixing in rep()
), and it can be re-used in the expect_equal($LITERAL, typeof(x))
family of lints too.
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 mark this as a TODO for now. we have a messy XPath that works for a limited number of cases, I'd like to generalize it if possible.
For reference here's the XPath for StringsAsFactorsLinter
(so we only look for STR_CONST
):
//expr[
expr[SYMBOL_FUNCTION_CALL[text() = 'data.frame']]
and expr[
(
STR_CONST
or (
expr[SYMBOL_FUNCTION_CALL[text() = 'c']]
and expr[STR_CONST]
and not(expr[SYMBOL or expr])
)
or expr[
SYMBOL_FUNCTION_CALL[text() = 'rep']
and (
following-sibling::expr[1][
STR_CONST
or (
expr[SYMBOL_FUNCTION_CALL[text() = 'c']]
and expr[STR_CONST]
and not(expr[SYMBOL or expr])
)
]
)
] or expr[SYMBOL_FUNCTION_CALL[
text() = 'character'
or text() = 'as.character'
or text() = 'paste'
or text() = 'sprintf'
or text() = 'format'
or text() = 'formatC'
or text() = 'prettyNum'
or text() = 'toString'
or text() = 'encodeString'
]]
)
and not(preceding-sibling::SYMBOL_SUB[1][text() = 'row.names'])
]
and not(SYMBOL_SUB[text() = 'stringsAsFactors'])
]
# Conflicts: # man/linters.Rd
# Conflicts: # man/linters.Rd # man/readability_linters.Rd
LGTM, needs a manual merge though, so not approving for now. |
# Conflicts: # man/linters.Rd
# Conflicts: # man/linters.Rd
Part of #962