Fix: Rpush magic doesn't support comma separated lists of variables #2736

Closed
wants to merge 1 commit into from

4 participants

@rmcgibbo

Implement @bfroehle's plan in #2733, using the split() implementation (no need to import separate re module)

@takluyver takluyver commented on the diff Jan 3, 2013
IPython/extensions/rmagic.py
@@ -259,7 +258,7 @@ def Rpull(self, line):
'''
args = parse_argstring(self.Rpull, line)
- outputs = args.outputs
+ outputs = ','.join(args.outputs).split(',')
@takluyver
IPython member

Can this line have an explanatory comment? I've worked out what it does now, but it took a few minutes of 'huh?' to get there.

@bfroehle
bfroehle added a note Jan 3, 2013

I don't think this will work, as ','.join(...) could result in 'a,,b,,c' and then .split(',') would make it ['a', '', 'b', '', 'c']. This has a lot of empty strings, which we do not want.

@takluyver
IPython member

What about the same trick we use in the other case? i.e. ' '.join(args.outputs).replace(',', ' ').split(). The no-argument form of split() treats multiple spaces as a single delimiter.

@takluyver
IPython member

(And I'd still like it to have an explanatory comment ;-) )

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

I'm generally -1 on this. It adds additional complexity (and maintenance burden) to the code, without other precedents. Nonetheless, if others think this is a good idea I won't stand in the way.

I did leave one inline comment showing why I think the current approach won't work correctly. Maybe instead do:

' '.join(args.output).replace(',', '').split()
@takluyver
IPython member

That's fair enough. The extra complexity is fairly minimal, though, and I think there is some precedent:

print a, b  # Python 2
global p, q
nonlocal x, y  # Python 3
@ellisonbg
IPython member

I would like to close this PR do to inactivity. I will open an issue to track progress. Are people OK with this plan?

@rmcgibbo

That's fine with me. I have other stuff on my plate.

@ellisonbg
IPython member

Closing this PR due to inactivity. Progress on this work will be tracked in issue #2733 Feel free to reopen this PR if work starts again.

@ellisonbg ellisonbg closed this Apr 10, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment