-
-
Notifications
You must be signed in to change notification settings - Fork 211
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
Add invisible param to export attribute #1025
Conversation
Looks fine in general, but I would of course like to know what @jjallaire thinks here. I also added an item to the checklist to make sure the corresponding vignette reflects it. |
I don't see any unit test about export params. Should I add tests about |
Good question. I don't think we have any, and should maybe table it. Would be nice to ascertain that we can turn visible on/off, that we can renamed (test could use Your PR is fine by me now. I will just wait for @jjallaire to glance at it too. So thanks for the PR and the quick refinements! |
Thanks for your kind suggestions! |
@@ -29,14 +34,14 @@ | |||
|
|||
* R/Attributes.R: Correct how cppFunction() deals with multiple | |||
depends arguments | |||
|
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.
nit: please avoid whitespace-only changes when possible; consider configuring your editor to only trim whitespace on the lines you've edited
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.
That hit me the other day too. So I just googled it for The One True Editor: https://www.emacswiki.org/emacs/DeletingWhitespace#toc11
Will give that a spin myself.
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.
Sorry for not noticing the auto formatting. I'll disable auto formatting for the project.
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.
LGTM!
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.
LGTM as well!
Closes #1024.
This PR add
invisible
param toexport
attribute so that ifRcpp::export(invisible=true)
is specified, the R wrapper will useinvisible(.Call(...))
instead of.Call(...)
.Checklist
R CMD check
still passes all tests