func_kw_complete for builtin and cython with embededsignature=True using docstring #2599

Merged
merged 9 commits into from Feb 4, 2013

Projects

None yet

4 participants

@piti118
np.array([1], su

has subok= in completion

min(k, k

has key= in completion list or

Cython embded signature works too ( http://wiki.cython.org/enhancements/compilerdirectives ) May be embded signature should be default in %%cython magic?

@cython.embdedsignature=True

def cython_func(a, mykwd=''):
    return mykwd

When you do

cython_func(a, my

has mykwd= in completion list

For Cython constructor, it's a little bit tricky since the constructor signature is put in class docstring not init. But, it's working

@cython.embdedsignature=True

class MyClass:
    def __init__(a, awesome_argument=1):
        pass
a = MyClass(a, awe

has awesome_argument= in the completion list too

@Carreau
IPython member

Hi,

Thanks this look really usefull.
I haven't tried it yet.

I'll be a little annoying and commend on code formatting, pep-8 and docstring inline.

@Carreau Carreau commented on an outdated diff Nov 18, 2012
IPython/core/completer.py
@@ -664,29 +665,66 @@ def python_matches(self,text):
return matches
+ def _default_arguments_from_docstring(self,doc):
+ """Parse first line of docstring of the
+ form 'min(iterable[, key=func])\n' to find
+ keyword argument names.
+ """
@Carreau
Carreau Nov 18, 2012

space after comma in function declaration.

Try to keep docstring 'one short line, one empty line, longer description'.

@Carreau Carreau commented on an outdated diff Nov 18, 2012
IPython/core/completer.py
@@ -664,29 +665,66 @@ def python_matches(self,text):
return matches
+ def _default_arguments_from_docstring(self,doc):
+ """Parse first line of docstring of the
+ form 'min(iterable[, key=func])\n' to find
+ keyword argument names.
+ """
+ if doc is None: return []
@Carreau
Carreau Nov 18, 2012

Could you put return []on a new line, it is easier to spot.

@Carreau Carreau commented on an outdated diff Nov 18, 2012
IPython/core/completer.py
@@ -664,29 +665,66 @@ def python_matches(self,text):
return matches
+ def _default_arguments_from_docstring(self,doc):
+ """Parse first line of docstring of the
+ form 'min(iterable[, key=func])\n' to find
+ keyword argument names.
+ """
+ if doc is None: return []
+ doc = doc.lstrip()
+ sio = StringIO.StringIO(doc)
+ #care only the firstline
+ #docstring can be long
+ line = sio.readline()
+ p = re.compile(r'^[\w|\s.]+\(([^)]*)\).*')
+ #'min(iterable[, key=func])\n' -> 'iterable[, key=func]'
+ sig = p.search(line)
+ if sig is None: return []
@Carreau Carreau commented on an outdated diff Nov 18, 2012
IPython/core/completer.py
getattr(obj,'__new__',None))
# for all others, check if they are __call__able
elif hasattr(obj, '__call__'):
- obj = obj.__call__
- # XXX: is there a way to handle the builtins ?
+ call_obj = obj.__call__
+
+ ret += self._default_arguments_from_docstring(
+ getattr(call_obj,'__doc__',''))
@Carreau
Carreau Nov 18, 2012

pep-8 here and there above.

@Carreau Carreau commented on the diff Nov 18, 2012
IPython/core/tests/test_completer.py
nt.assert_in('b=', matches)
- s, matches = c.complete(None,'myfunc(a="escaped\\")string",b')
+ s, matches = c.complete(None, 'myfunc(a="escaped\\")string",b')
@Carreau
Carreau Nov 18, 2012

see, you already fixed pep-8 here:-)

@bfroehle bfroehle and 1 other commented on an outdated diff Nov 18, 2012
IPython/core/completer.py
@@ -664,29 +665,66 @@ def python_matches(self,text):
return matches
+ def _default_arguments_from_docstring(self,doc):
+ """Parse first line of docstring of the
+ form 'min(iterable[, key=func])\n' to find
+ keyword argument names.
+ """
+ if doc is None: return []
+ doc = doc.lstrip()
+ sio = StringIO.StringIO(doc)
+ #care only the firstline
+ #docstring can be long
+ line = sio.readline()
@bfroehle
bfroehle Nov 18, 2012

Is this somehow better than line = doc.lstrip().splitlines()[0] ?

@piti118
piti118 Nov 18, 2012

splitlines, i believe, read the whole docstring which could be long. While StringIO, i presume, only look until it found the first newline.

@bfroehle
bfroehle Nov 18, 2012

Yes, but even on a giant docstring it's only going to be a few microseconds. So it's probably not worth the optimization. The "fastest" approach would probably be something like:

line = re.split(r'[\r\n]', doc, 1)[0]
@piti118
piti118 Nov 19, 2012

Which one do you want me to change it to? I'm good with any of the alternative you suggest. In my opinion, they are all the same in term of readability. Everyone can figure out that all of these means reading the first line (modulo some fine detail).

@bfroehle bfroehle commented on an outdated diff Nov 18, 2012
IPython/core/completer.py
+ """
+ if doc is None: return []
+ doc = doc.lstrip()
+ sio = StringIO.StringIO(doc)
+ #care only the firstline
+ #docstring can be long
+ line = sio.readline()
+ p = re.compile(r'^[\w|\s.]+\(([^)]*)\).*')
+ #'min(iterable[, key=func])\n' -> 'iterable[, key=func]'
+ sig = p.search(line)
+ if sig is None: return []
+ # iterable[, key=func]' -> ['iterable[' ,' key=func]']
+ sig = sig.groups()[0].split(',')
+ #use this if you want iterable to show up too
+ #q = re.compile('[\s|\[]*(\w+)(?:\s*=?\s*.*)')
+ q = re.compile('[\s|\[]*(\w+)(?:\s*=\s*.*)')
@bfroehle
bfroehle Nov 18, 2012

This should be precompiled (in __init__) and saved as self.docstring_arguments_re or similar.

@piti118

It has been a while. Is there anything else that needs to be done for this PR?

@minrk minrk and 2 others commented on an outdated diff Jan 18, 2013
IPython/core/completer.py
@@ -664,29 +671,67 @@ def python_matches(self,text):
return matches
+ def _default_arguments_from_docstring(self, doc):
+ """Parse first line of docstring for call signature.
+
+ Docstring should be of the form 'min(iterable[, key=func])\n'.
+ It can also parse cython docstring of the form
+ 'Minuit.migrad(self, int ncall=10000, resume=True, int nsplit=1)'.
+ """
+ if doc is None:
+ return []
+ sio = StringIO.StringIO(doc.lstrip())
+ #care only the firstline
+ #docstring can be long
+ line = sio.readline()
@minrk
minrk Jan 18, 2013

why StringIO / readline, and not just doc.splitlines()[0]?

@piti118
piti118 Jan 18, 2013

either way works and equally readable. I just don't want to read the whole string.

@piti118
piti118 Jan 18, 2013

I mean I can change it if it's the thing that blocks this PR. I don't have a hard opinion on this one. Anything that work is fine by me. I recall someone asked the same thing earlier too.

@bfroehle
bfroehle Jan 18, 2013

Regardless can you shrink the command to one line?

line = StringIO.StringIO(doc.lstrip()).readline()

or

line = doc.lstrip().splitlines()[0]

Personally I think the second option is more readable. Do beware that the two options differ in a final '\n' character.

In terms of timings, I don't think we need to worry about micro-optimizing this.

In [16]: doc = matplotlib.pyplot.plot.__doc__

In [17]: len(doc.splitlines())
Out[17]: 157

In [18]: %timeit line1 = StringIO.StringIO(doc.lstrip()).readline()
100000 loops, best of 3: 3.73 us per loop

In [19]: %timeit line2 = doc.lstrip().splitlines()[0]
10000 loops, best of 3: 23.1 us per loop
@piti118

I also rebase... hopefully that merge button will appear.

@Carreau
IPython member

Looks like all the comments have been fixed.
+1 to merge.

@minrk minrk commented on the diff Feb 4, 2013
IPython/core/tests/test_completer.py
+def test_default_arguments_from_docstring():
+ doc = min.__doc__
+ ip = get_ipython()
+ c = ip.Completer
+ kwd = c._default_arguments_from_docstring(
+ 'min(iterable[, key=func]) -> value')
+ nt.assert_equal(kwd, ['key'])
+ #with cython type etc
+ kwd = c._default_arguments_from_docstring(
+ 'Minuit.migrad(self, int ncall=10000, resume=True, int nsplit=1)\n')
+ nt.assert_equal(kwd, ['ncall', 'resume', 'nsplit'])
+ #white spaces
+ kwd = c._default_arguments_from_docstring(
+ '\n Minuit.migrad(self, int ncall=10000, resume=True, int nsplit=1)\n')
+ nt.assert_equal(kwd, ['ncall', 'resume', 'nsplit'])
+
@minrk
minrk Feb 4, 2013

these two tests seem to be identical. Based on the comments, I presume one is meant to be lacking whitespace separating args? Might have gotten a bit overzealous pep8ifying here.

@piti118
piti118 Feb 4, 2013

which two?
The last two differs by the white spaces in the front. Arguably we can just keep the second one.

@minrk
minrk Feb 4, 2013

ah, I see. I missed the front. Nevermind.

@minrk
IPython member

Gotcha, I agree with @Carreau, merging away.

@minrk minrk merged commit 918365c into ipython:master Feb 4, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment