Conversation
|
|
||
| # Check if argument list is empty | ||
| if(length(L) == 0) { | ||
| stop(paste("No arguments passed to animint")) |
There was a problem hiding this comment.
Could you please make the error message more informative? Please tell the user how to fix the problem.
something like "No arguments passed to animint, but arguments should be ggplots (1 or more) and options (0 or more)"
Also please remove paste as it is not necessary.
tdhock
left a comment
tdhock
left a comment
There was a problem hiding this comment.
good start, please address comments and then I will merge.
| }) | ||
|
|
||
| test_that("animint() is an error", { | ||
|
|
There was a problem hiding this comment.
please remove empty line inside test_that block.
empty lines like this make it more difficult to run the whole block of code interactively. (that can be done with C-c C-c in emacs, etc)
| test_that("animint() is an error", { | ||
| expect_error({ | ||
| animint() | ||
| }, "No arguments passed to animint. Arguments should include ggplot(1 or more) and options(0 or more)") |
There was a problem hiding this comment.
the old version is more correct english (although I would say ggplots instead of ggplot), can you please revert?
why did you make this change?
did you know about expect_error(something, "message", fixed=TRUE) ?
There was a problem hiding this comment.
Reverted the error message to the old one and used fixed=TRUE. I made this change because I thought it would be better to avoid '()' in the error message sentence. I had used escape qualifiers to do the pattern matching in the test and didn't know about fixed=TRUE !
Use fixed=TRUE instead of escape sequence
|
looks great thanks! |
2 participants
Follow up to PR116
Added my name as contributor in DESCRIPTION
Increased version
Added a NEWS item
Added a fix for empty animint() and a test. I missed to commit the test first and will do it from the next time.