Magic arguments #199

Merged
merged 3 commits into from Mar 24, 2011

3 participants

@rkern

Here are the magic arguments decorators. I haven't modified any of the existing magics.

@fperez
IPython member

Great, many thanks! Only one minor note: don't use shlex.split, it's borked with unicode input. Instead, use this:

https://github.com/ipython/ipython/blob/master/IPython/utils/process.py#L106

It's kind of ugly that we actually have two shlex.split replacements, the second one is:

https://github.com/ipython/ipython/blob/master/IPython/core/completerlib.py#L58

That one does extra stuff to handle incomplete lines. Use whichever you prefer, but definitely don't rely on the plain one.

Other than that, this looks fantastic. Many thanks!

@rkern

Really, neither of those should be using sys.stdin.encoding. They can probably encode using utf-8 and then decoding the tokens on the way out. That shouldn't break up any of the multi-byte sequences. Most robustly, we could just include our own version that uses a unicode-aware StringIO.

@fperez
IPython member

If you can add a more robust one in this same request, by all means go for it. I know this has caused real problems in the past, and we're likely to use it even more in magics, so the more robust it is the better.

@ellisonbg
IPython member

Is this pull request ready to be merged?

@rkern

The I don't like either of the shlex.split replacements, but I guess I might as well use one and be done with it. I'll finish it up this weekend.

rkern added some commits Jan 26, 2011
@rkern rkern BUG: when given unicode inputs, arg_split should return unicode outpu…
…ts. Always use utf-8 to encode the string instead of relying on sys.stdin.encoding, which may not be able to accept the full range of Unicode characters. When given unicode strings, arg_split is probably not receiving input from a terminal.
6aff23d
@rkern rkern BUG: Use arg_split instead of shlex.split e0e8fbb
@rkern

I updated arg_split to be more robust when it comes to unicode inputs. I updated magic-arguments to use it. It should be ready to merge now.

@fperez fperez commented on the diff Mar 24, 2011
IPython/core/magic_arguments.py
+ help_text = parser.format_help()
+ if help_text.startswith('usage: '):
+ help_text = help_text.replace('usage: ', '%', 1)
+ else:
+ help_text = '%' + help_text
+
+ # Replace the magic function's docstring with the full help text.
+ magic_func.__doc__ = help_text
+
+ return parser
+
+
+def parse_argstring(magic_func, argstring):
+ """ Parse the string of arguments for the given magic function.
+ """
+ args = magic_func.parser.parse_argstring(argstring)
@fperez
IPython member
fperez added a line comment Mar 24, 2011

We can just return the value straight away, no need to create that intermediate variable.

Don't worry though: the pull request is in almost perfect shape, so I'll do the cleanup as part of the merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@fperez fperez commented on the diff Mar 24, 2011
IPython/core/tests/test_magic_arguments.py
+
+@magic_arguments()
+@argument('-f', '--foo', help="an argument")
+def magic_magic_foo(self, args):
+ """ A docstring.
+ """
+ return parse_argstring(magic_magic_foo, args)
+
+@magic_arguments()
+@argument('-f', '--foo', help="an argument")
+def foo(self, args):
+ """ A docstring.
+ """
+ return parse_argstring(foo, args)
+
+def test_magic_arguments():
@fperez
IPython member
fperez added a line comment Mar 24, 2011

we'll convert these to use @parametric. For all its flaws, it's still better than the impossible to debug situation produced by nose's yield tests, which return an alternate stack (the one from nose) when they fail.

In the future, please use @parametric for these situations.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@fperez fperez merged commit e0e8fbb into ipython:master Mar 24, 2011
@fperez
IPython member

Thanks a bunch, Robert! Sorry for the delay in merging.

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