-
Notifications
You must be signed in to change notification settings - Fork 298
Implement is_complete_request #102
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
Conversation
R/kernel.r
Outdated
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 generally add things to a list by doing content$indent = "". Both work, I don't know which is considered more idiomatic R.
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 to google for it and got it from here: http://stackoverflow.com/a/2436960/1380673 :-)
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 think c is clearer in that you want to add a new entry instead of updating one, but since the variable is defined 2 lines above it doesn’t matter
|
This is looking good to me - @flying-sheep, can you take a quick look at it before we merge? |
R/kernel.r
Outdated
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.
this is exported in evaluate, so why three “:::”? does this even work? i thought exported stuff requires exactly two “::”?
/edit: you can access it with :::, but that implies unsafe private access.
please add #' @importFrom evaluate parse_all somewhere (e.g. at the very top of the file) and drop the evaluate::: from this line.
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.
done, hopefully :-)
|
pushed, but untested (didn't install due to jsonline and have to run :-( ) |
|
Thanks, I think that looks good. I'll merge and run roxygenise to update NAMESPACE |
Implement is_complete_request
Closes: #94
This is tested against knitpy with a R kernel document, so it seems it works...
PS: @takluyver senses are good... :-)