Bug cpp11 decorators #197
Bug cpp11 decorators #197
Conversation
There was a problem hiding this comment.
In addition to your changes (which look good!) I think we should remove the \dontrun{} block around the example at
Line 25 in 532cd3f
I initially used \dontrun{} so users wouldn't be loading a shared library when they ran the examples, but I now think that isn't really a problem and it would have caught this regression during R CMD check.
Also possibly we should change Downloading in the example to Processing, otherwise when you run the examples it looks like you are downloading something, which might scare people.
R/register.R
Outdated
| @@ -279,7 +279,9 @@ get_cpp_register_needs <- function() { | |||
|
|
|||
| check_valid_attributes <- function(decorations) { | |||
|
|
|||
| bad_decor <- !decorations$decoration %in% c("cpp11::register", "cpp11::init") | |||
| bad_decor <- grepl("cpp11::", decorations$decoration) & (!decorations$decoration %in% c("cpp11::register", | |||
There was a problem hiding this comment.
startsWith() would be slightly preferred here.
If you do want to use grepl() you need to anchor it to the start of the string, e.g. grepl("^cpp11::")
There was a problem hiding this comment.
Yep you're right! Thanks for the review
Co-authored-by: Jim Hester <james.f.hester@gmail.com>
R/register.R
Outdated
| bad_decor <- startsWith(decorations$decoration, "cpp11::") & (!decorations$decoration %in% c("cpp11::register", | ||
| "cpp11::init", | ||
| "cpp11::linking_to")) |
There was a problem hiding this comment.
Is this still lining up like you wanted?
It might be worth doing the line breaking a bit differently, e.g.
| bad_decor <- startsWith(decorations$decoration, "cpp11::") & (!decorations$decoration %in% c("cpp11::register", | |
| "cpp11::init", | |
| "cpp11::linking_to")) | |
| bad_decor <- startsWith(decorations$decoration, "cpp11::") & | |
| (!decorations$decoration %in% c("cpp11::register", "cpp11::init", "cpp11::linking_to")) |
|
|
Also added new tests for linking_to
fixes #193