Extend conjunct_expectation_linter to include stopifnot(), rename to conjunct_test_linter #1011
Conversation
|
Let's merge the linters right away. I think the old name still fits. |
To me
|
R/conjunct_stopifnot_linter.R
Outdated
|
|
||
| xml <- source_file$full_xml_parsed_content | ||
|
|
||
| xpath <- " |
There was a problem hiding this comment.
I also need to add a TODO for R>=4.0 not to lint in this case:
stopifnot(`Descriptive reason` = A && B)
possibly optionally
There was a problem hiding this comment.
Hmm. Could still be written as
stopifnot(
`Descriptive reason, A failed` = A,
`Descriptive reason, B failed` = B
)
There was a problem hiding this comment.
Right, but it can be tedious... consider
stopifnot(
`x is a logical scalar` = length(x) == 1 && is.logical(x) && !is.na(x),
`y is a logical scalar` = length(y) == 1 && is.logical(y) && !is.na(y),
)
So I think it should be optional.
note that this is equivalent to allowing
assertthat::assert_that(length(x) == 1, is.logical(x), !is.na(x), msg="x is a logical scalar")
instead of requiring several assert_that() calls with a unique msg
|
Fine with that as well.
Fine by me. |
Let's add a TODO... I'm not as familiar with |
|
Nope, |
| @@ -17,9 +17,11 @@ conjunct_expectation_linter <- function() { | |||
|
|
|||
| xml <- source_file$full_xml_parsed_content | |||
|
|
|||
| # TODO(#1016): include assert_that() for consideration here too | |||
There was a problem hiding this comment.
That would be just or text() = 'assert_that' in the XPath
There was a problem hiding this comment.
right... plus tests. better not to clutter this PR with that
Part of #962
Similar naming issue to our
TrueFalseAndConditionLinterwhich becameconjunct_expectation_linter. We might consider merging them into just one linter.