Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Fix: Rpush magic doesn't find local variables and doesn't support comma separated lists of variables #2698

Closed
wants to merge 3 commits into
from

Conversation

Projects
None yet
3 participants
Contributor

rmcgibbo commented Dec 18, 2012

Described in issue #2550. This is a simple fix. It has tests.

Note. I submitted this PR a while ago, but then messed up my git. I did a hard reset and tried again. The old PR was #2552.

@bfroehle bfroehle commented on an outdated diff Dec 18, 2012

IPython/extensions/rmagic.py
@@ -512,7 +527,7 @@ def R(self, line, cell=None, local_ns=None):
# execute the R code in a temporary directory
tmpd = tempfile.mkdtemp()
- self.r('png("%s/Rplots%%03d.png",%s)' % (tmpd.replace('\\', '/'), png_args))
@bfroehle

bfroehle Dec 18, 2012

Contributor

This change seems unintentional.

@bfroehle bfroehle commented on an outdated diff Dec 18, 2012

IPython/extensions/rmagic.py
@@ -590,4 +605,4 @@ def R(self, line, cell=None, local_ns=None):
def load_ipython_extension(ip):
"""Load the extension in IPython."""
- ip.register_magics(RMagics)
@bfroehle

bfroehle Dec 18, 2012

Contributor

This change seems unintentional.

@bfroehle bfroehle commented on an outdated diff Dec 18, 2012

IPython/extensions/tests/test_rmagic.py
@@ -79,4 +96,4 @@ def test_rmagic_localscope():
KeyError,
ip.run_line_magic,
"R",
- "-i var_not_defined 1+1")
+ "-i var_not_defined 1+1")
@bfroehle

bfroehle Dec 18, 2012

Contributor

Please end the file with a newline, if possible.

Contributor

bfroehle commented Dec 18, 2012

Looks pretty good. I left a few comments inline and will be glad to merge when those are addressed.

Contributor

rmcgibbo commented Dec 18, 2012

Whoops. I should have seen those. Thanks! Fixes made.

@takluyver takluyver commented on the diff Dec 18, 2012

IPython/extensions/rmagic.py
@@ -199,10 +201,23 @@ def Rpush(self, line):
Out[11]: array([ 6.23333333])
'''
+ # split the line into fields separated by commas or spaces
+ # i.e. 'a,b', 'a b', 'a, b', 'a ,b', 'a , b' should all be split
+ # to ['a', 'b']
+ inputs = re.split(r'\s*[,\s]\s*', line)
@takluyver

takluyver Dec 18, 2012

Owner

Is line.replace(',', ' ').split() more readable?

@bfroehle

bfroehle Dec 18, 2012

Contributor

Or re.split(r'[\s,]+', line).

Owner

takluyver commented Dec 18, 2012

Apart from that question, it looks OK. The Travis failure is just Travis failing, not a code problem.

Contributor

bfroehle commented Jan 2, 2013

I've closed this pull request in favor of #2731. Note that the pull request there does NOT add support for separating the variable names with spaces. That can come in a later pull request (which also fixes %Rpull).

@bfroehle bfroehle closed this Jan 2, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment