Throw error when same argument is passed to aes and geom #119
Conversation
|
please add a NEWS item and increase version number. |
|
Can you please add a test for geom(aes(alpha),alpha_off) ? |
Added test for geom(aes(alpha),alpha_off). This required additional changes to match _off parameters. Also, moved the validation checks to geom-.r from layer.r |
R/geom-.r
Outdated
| result <- sub(pattern, "", input_string) | ||
| return(result) | ||
| } else { | ||
| return("") |
There was a problem hiding this comment.
it is simpler to not use return
also this whole function could be replaced by grep(input_string, pattern, value=TRUE)
There was a problem hiding this comment.
Thanks for the suggestion. I rewrote the check using grep and sub and the whole function is not required anymore
| ) | ||
| expect_error({ | ||
| animint2dir(viz, open.browser=FALSE) | ||
| }, "Same argument cannot be passed to both aes and geom. Argument passed to both aes and geom: alpha") |
There was a problem hiding this comment.
can you please change the error message? it is not true that alpha was passed to both aes and geom, because alpha_off was passed to geom.
Also it would be good to explain why the same argument cannot be used / how to fix: the visual property needs only be defined in one place, so if the alpha should be different for each rendered geom, but not depend on selection state, then use aes(alpha); but if the alpha should depend on the selection state then use geom(alpha,alpha_off).
and actually for this test to make more sense with alpha_off, you would need to add clickSelects="some_selection_variable" so that the alpha_off could actually be used.
In fact could you please add another test for an error message when alpha_off is used in a geom without clickSelects? that should be a different error (alpha_off defines the opacity of geoms with clickSelects, but are not currently selected, so please fix by removing alpha_off or adding clickSelects)
There was a problem hiding this comment.
I updated the error message to distinguish between alpha and alpha_off. Is the new error message better to understand for the user ?
Added a new test without clickSelects and alpha_off and verified that the correct warning is displayed. When aplha_off is used without clickSelects, currently a warning is thrown. Does it make more sense to throw an error instead ?
There was a problem hiding this comment.
Currently if there are multiple errors, one error masks the others since the compiler stops upon an error. Maybe this can be modified to throw multiple errors before stopping when its possible to continue compilation. gcc/llvm do that and continue compilation to discover additional errors
| ) | ||
| expect_warning({ | ||
| animint2dir(viz, open.browser=FALSE) | ||
| }, "geom1_path_plot has alpha_off which is not used because this geom has no clickSelects; please specify clickSelects or remove alpha_off") |
|
thanks very much for this useful contribution |
Throw error when same argument is passed to aes and geom
Closes #110
Because of the change, the test "empty data overrides plot defaults" would always error out since it aes(mpg, wt) is passed along with x,y to plot