Skip to content
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

bind last output to .Last.value #406

Merged
merged 2 commits into from Sep 12, 2016
Merged

Conversation

randy3k
Copy link
Contributor

@randy3k randy3k commented Sep 10, 2016

close #405

Thanks @flying-sheep for pointing out the relevant code. Now .Last.value works as expected (even within a cell).

screen shot 2016-09-11 at 12 27 43 am

@randy3k
Copy link
Contributor Author

randy3k commented Sep 10, 2016

I think the test failure is not relevant, but there is a note

Found the following possibly unsafe calls:
File ‘IRkernel/R/utils.r’:
  unlockBinding(".Last.value", .BaseNamespaceEnv)

Basically, it says unlocking .Last.value is not safe, but I don't know there is a way to do it safely. Perhaps assigning the last value to a global variable like .jupyter.last_value rather than base::.Last.value?

@flying-sheep
Copy link
Member

flying-sheep commented Sep 11, 2016

well, the docs say that it lives in the base namespace, so you’re doing the right thing.

we’re basically replacing the frontend of the whole R runtime, so it’s expected that we do a few things that are considered “potentially unsafe”.

and of course your changes cause the test failure: we just test for the number of lines the “NOTE” has… we could do this better, but for now we have to update and account for every single line. as you can see it went from 4→9. what lines gets added?

allow .Last.value for non-visible commands

fixup
@randy3k
Copy link
Contributor Author

randy3k commented Sep 11, 2016

I have updated the tests, 5 out of 7 of the jobs pass except the R-devel versions on linux. I have no clues how to fix them..

@flying-sheep
Copy link
Member

flying-sheep commented Sep 12, 2016

the devel test fails aren’t yours, they’re ours (turns out our tests didn’t actually run for some time)

it’s not “8 lines from the ‘attach’ NOTE”, though. it’s still 4 for that one and 4 for your new one.

fixup! fix number of lines of note

fixup! fix number of lines of note
@randy3k
Copy link
Contributor Author

randy3k commented Sep 12, 2016

I fixed the comments in .travis.sh

@flying-sheep
Copy link
Member

great. i’ll wait for the test results and merge

@flying-sheep flying-sheep merged commit 91108bf into IRkernel:master Sep 12, 2016
@randy3k randy3k deleted the last_value branch September 12, 2016 14:47
@flying-sheep
Copy link
Member

oh, and of course: thank you 😄

@randy3k
Copy link
Contributor Author

randy3k commented Sep 12, 2016

😀
You are welcome

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.

.Last.value does not work in IRkernel
2 participants