Skip to content

Conversation

@jankatins
Copy link
Contributor

Closes: #94

This is tested against knitpy with a R kernel document, so it seems it works...

PS: @takluyver senses are good... :-)

@jankatins jankatins mentioned this pull request Apr 25, 2015
10 tasks
R/kernel.r Outdated
Copy link
Member

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.

Copy link
Contributor Author

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 :-)

Copy link
Member

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

@takluyver takluyver added this to the 0.3 milestone Apr 25, 2015
@takluyver
Copy link
Member

This is looking good to me - @flying-sheep, can you take a quick look at it before we merge?

R/kernel.r Outdated
Copy link
Member

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done, hopefully :-)

@jankatins
Copy link
Contributor Author

pushed, but untested (didn't install due to jsonline and have to run :-( )

@takluyver
Copy link
Member

Thanks, I think that looks good. I'll merge and run roxygenise to update NAMESPACE

takluyver added a commit that referenced this pull request Apr 26, 2015
Implement is_complete_request
@takluyver takluyver merged commit d630e76 into IRkernel:master Apr 26, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

"Got unhandled msg_type:" "is_complete_request"

3 participants