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 special env to take r shadowing functions #324
Changes from 6 commits
18faa3a
7e10b4b
0800799
e1a9579
7e2265f
d02847d
a081831
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10,3 +10,4 @@ example-notebooks/.*$ | |
^\.travis.yml$ | ||
Dockerfile | ||
test_ir.py | ||
cran-comments.md |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,18 @@ | ||
# Everthing related to the environment which takes functions which shadow base R functions. | ||
# This is needed to build in our own needs, like properly shutting down the kernel | ||
# when `quit()` is called. | ||
|
||
# The real env is created and attach'ed in the main() via the call to init_shadowenv()! | ||
init_shadowenv <- function(){ | ||
.irk.shadowenv <- attach(NULL, name = "jupyter:irkernel") | ||
|
||
add_to_user_searchpath <- function(name, FN, attrs = list()) { | ||
assign(name, FN, .irk.shadowenv) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can you explain why don’t you use the environment name as i suggested? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Probably because I don't know suche a method :-) You mean there is something like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. no: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. because I didn't know that this was possible :-) Seeing this, I think it is even possible to not init anything but just place these functions in the package... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. good idea! |
||
} | ||
|
||
# add the accessors to the shadow env to that env, so they are actually accessable | ||
# from everywhere... | ||
add_to_user_searchpath(".irk.get_shadowenv", function() {.irk.shadowenv}) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's another "habit" (apart from the smartquotes... :-)) of mine which I don't want to break: all codeblocks belong into brackets so that in the next "enter", the functions still works and does not need to be edited to get such brackets. -> If there is error to have these brackets here, I would like to keep them... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it’s just R style. functions with one statement have no brackets. e do this everywhere, in every project, and others do as well. it’s OK not to do it in cases like line 9 (assigning a function to a name), but when using a function as parameter, those braces just serve to make noise. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
We really have a different opinion on what looks good and what not :-) It really pains me to remove that... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. well, as said: it’s like that everywhere in the R world. but if you’re OK with me some day forgetting and removing the braces, then leave it in 😄 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I solved by making a normal functions form it and there I added braces... BTW: gcc just got warnings to check for this (pseudo code):
This would not have happend / needed if everybody would just always use braces :-) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, my understanding is that - at least in C - omitting the parentheses is considered slightly bad practise. |
||
add_to_user_searchpath(".irk.add_to_user_searchpath", add_to_user_searchpath) | ||
|
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -115,7 +115,7 @@ quit = function(save = 'default', status = 0, runLast = TRUE) { | |
if (!is.null(.GlobalEnv$.Last.sys)) .GlobalEnv$.Last.sys() | ||
} | ||
if (save) NULL # TODO: actually save history | ||
payload <<- c(payload, list(list(source = 'ask_exit', keepkernel = FALSE))) | ||
payload <<- c(.self$payload, list(list(source = 'ask_exit', keepkernel = FALSE))) | ||
}, | ||
|
||
handle_error = function(e) { | ||
|
@@ -215,9 +215,9 @@ execute = function(request) { | |
err <<- list() | ||
|
||
# shade base::quit | ||
assign('quit', .self$quit, envir = .GlobalEnv) | ||
assign('q', .self$quit, envir = .GlobalEnv) | ||
.irk.add_to_user_searchpath('quit', .self$quit) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. move to main? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think there are a lot of things which are static between code executions, one could argue that these belong to the init method of the executer. -> later PR to cleanup that? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. sure, why not! 😄 |
||
.irk.add_to_user_searchpath('q', .self$quit) | ||
|
||
# find out stack depth in notebook cell | ||
# TODO: maybe replace with a single call on first execute and rest reuse the value? | ||
tryCatch(evaluate( | ||
|
@@ -250,12 +250,12 @@ execute = function(request) { | |
# Workaround to warn user when code contains potential problematic code | ||
# https://github.com/IRkernel/repr/issues/28#issuecomment-208810772 | ||
# See https://github.com/hadley/evaluate/issues/66 | ||
if(.Platform$OS.type == 'windows') { | ||
if (.Platform$OS.type == 'windows') { | ||
# strip whitespace, because trailing newlines would trip the test... | ||
code <- gsub('^\\s+|\\s+$', '', request$content$code) | ||
real_len <- nchar(code) | ||
r_len <- nchar(paste(capture.output(cat(code)), collapse = '\n')) | ||
if (real_len != r_len){ | ||
if (real_len != r_len) { | ||
msg = c('Your code contains a unicode char which cannot be displayed in your ', | ||
'current locale and R will silently convert it to an escaped form when the ', | ||
'R kernel executes this code. This can lead to subtle errors if you use ', | ||
|
@@ -264,7 +264,6 @@ execute = function(request) { | |
send_error_msg(paste(msg, collapse = '\n')) | ||
} | ||
} | ||
|
||
tryCatch( | ||
evaluate( | ||
request$content$code, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -166,7 +166,7 @@ abort_queued_messages = function() { | |
c(.pbd_env$ZMQ.PO$POLLIN), # type | ||
0) # zero timeout, only what's already there | ||
log_debug('abort loop: after poll') | ||
if(bitwAnd(zmq.poll.get.revents(1), .pbd_env$ZMQ.PO$POLLIN)) { | ||
if (bitwAnd(zmq.poll.get.revents(1), .pbd_env$ZMQ.PO$POLLIN)) { | ||
log_debug('abort loop: found msg') | ||
abort_shell_msg() | ||
} else { | ||
|
@@ -343,13 +343,13 @@ run = function() { | |
# the easiest seems to be to handle this in a big if .. else if .. else | ||
# clause... | ||
# https://github.com/IRkernel/IRkernel/pull/266 | ||
if(bitwAnd(zmq.poll.get.revents(1), .pbd_env$ZMQ.PO$POLLIN)) { | ||
if (bitwAnd(zmq.poll.get.revents(1), .pbd_env$ZMQ.PO$POLLIN)) { | ||
log_debug('main loop: hb') | ||
hb_reply() | ||
} else if(bitwAnd(zmq.poll.get.revents(2), .pbd_env$ZMQ.PO$POLLIN)) { | ||
} else if (bitwAnd(zmq.poll.get.revents(2), .pbd_env$ZMQ.PO$POLLIN)) { | ||
log_debug('main loop: shell') | ||
handle_shell() | ||
} else if(bitwAnd(zmq.poll.get.revents(3), .pbd_env$ZMQ.PO$POLLIN)) { | ||
} else if (bitwAnd(zmq.poll.get.revents(3), .pbd_env$ZMQ.PO$POLLIN)) { | ||
log_debug('main loop: control') | ||
handle_control() | ||
} else { | ||
|
@@ -381,6 +381,13 @@ main <- function(connection_file = '') { | |
connection_file <- commandArgs(TRUE)[[1]] | ||
} | ||
log_debug('Starting the R kernel...') | ||
# Create the shadow env here | ||
# This has to be here so the on.exit can detach the env when main is finished | ||
# = available for the whole lifetime of the kernel. | ||
init_shadowenv() | ||
on.exit({ | ||
detach("jupyter:irkernel") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
}) | ||
kernel <- Kernel$new(connection_file = connection_file) | ||
kernel$run() | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,33 @@ | ||
## Release summary | ||
|
||
This package is new on CRAN. It contains the R kernel for the Jupyter | ||
ecosystem. The kernel executes R code, which the frontend (Jupyter Notebook or | ||
other frontends) submits to the kernel via the network. | ||
|
||
## Test environments | ||
|
||
* local Win7 64bit install, R 3.2.5, R 3.3.0 and r-devel (3.4) | ||
* Ubuntu 12.04 (on travis-ci), oldrelease, release, and r-devel | ||
|
||
## R CMD check results | ||
|
||
There are no WARNINGs and 1 NOTEs. | ||
|
||
Note: | ||
|
||
* Found the following calls to attach(): | ||
File 'IRkernel/R/environment.r': | ||
attach(NULL, name = "jupyter:irkernel") | ||
See section 'Good practice' in '?attach'. | ||
|
||
We use this additional environment to add functions so that regular "stuff" | ||
like `quit()` works in the IRkernel environment (in this case to shutdown the | ||
kernel). We added the "good practice" `on.exit` call to `detach` in R\kernel.r -> | ||
`main()` so that this environment is available as long as the R kernel is running.. | ||
|
||
## Downstream dependencies | ||
|
||
It is assumed that there won't be any code dependencies on this package, as it | ||
implements an application without any API apart from the startup function and the | ||
implemented Jupyter Messaging spec. It's usually started as | ||
`R --slave -e IRkernel::main() --args {connection_file}`. |
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.
){
→) {
""
→''
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 should really invest some time into setting up some precommit hook...
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 tried to create some lintr rules, but somehow can’t get it to run: r-lib/lintr#139
no idea what i do wrong