-
Notifications
You must be signed in to change notification settings - Fork 323
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
feat: mock_output_sequence()
similar to {mockery}'s multiple return values
#2061
Conversation
The failure on Ubuntu seems unrelated to the changes ══ Failed tests ════════════════════════════════════════════════════════════════
── Error ('test-local.R:38:3'): can override translation of error messages ─────
<notSubsettableError/error/condition>
Error in `mean[[1]]`: object of type 'closure' is not subsettable |
R/mock2-helpers.R
Outdated
i = "You can set {.arg recycle} to {.code TRUE}." | ||
)) | ||
} | ||
value <- rep_len(values, length.out = i)[[i]] |
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.
It'd be a little more efficient to do this with modular arithmetic, something like i <- (i - 1) %% length(values) + 1
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.
Aaaah I had not found this elegant solution. Code now refactored!
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 had been using the modulo operator with if and else... 🤪 )
37aa592
to
f20a031
Compare
R/mock2-helpers.R
Outdated
#' recycled_mocked_sequence() | ||
#' recycled_mocked_sequence() | ||
#' @family mocking | ||
mock_output_sequence <- function(values, recycle = FALSE) { |
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.
Should we use ...
instead of values
?
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.
To be sure I understand: do you mean that instead of writing mock_output_sequence(c("blop", "bla"))
the user would write mock_output_sequence("blop", "bla")
?
And using rlang's "Dynamic dots features"?
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.
Yes and yes 😄
tests/testthat/test-mock2-helpers.R
Outdated
expect_snapshot(mocked_sequence(), error = TRUE) | ||
}) | ||
|
||
test_that("mock_output_sequence() works -- force", { |
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 tried to write a test that'd fail if I didn't use force()
but it doesn't fail so I removed force()
. Maybe because of list2()
? Or maybe because I missed something obvious. 😅
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.
Yes, it's no longer needed because list2
forces all the values in ...
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.
So you can remove this test IMO
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.
Ok, done
Thanks! |
#1265 (comment)