[qtconsole] take %,%% prefix into account for completion #1944

Carreau · 2012-06-13T13:57:35Z

fperez · 2012-06-13T18:45:55Z

I'm busy today, but if anyone can review and test this, it would be great. This is an important fix to get in for 0.13, so it would be better to have it before the first beta (which I'm delaying to give @ellisonbg some time to review some NB stuff)

takluyver · 2012-06-13T18:53:30Z

I've run it and verified that it fixes the bug.

fperez · 2012-06-13T18:56:58Z

Great, go for it then. It's an important one; I have to run now.

takluyver · 2012-06-13T18:58:31Z

IPython/frontend/qt/console/console_widget.py

+            prefix = u'%'
+    if prefix :
+        prefix_filter = lambda x: u"".join(x.split(u'%', 2))
+        items = map(prefix_filter, items)


I feel like there ought to be a neater way to do this. How about [s.lstrip('%') for s in items]. Admittedly that would remove any number of leading %, but if we ever do have something with 3, that's probably the behaviour we want anyway.

we do lstrip(ESC_MAGIC) elsewhere in inspection, so this should be fine.

takluyver · 2012-06-13T19:59:51Z

Doing it in the kernel would require an addition to the messaging protocol. If we think that's the right way to do it, I think we should apply this quick fix for 0.13, give or take some small tweaks, and revisit it after the release. I'm happy that it works with the code we have today, and I don't think we want to start refactoring completion code just now.

Carreau · 2012-06-13T20:04:12Z

I thing that i can extract the prefixes, reverse them, and find the common sufixes of the prefix,that should be doable.

takluyver · 2012-06-13T20:14:04Z

Not sure I follow that ;-)

fixes ipython#1767

Carreau · 2012-06-13T20:34:10Z

Forced push to have only one commit, should take into account any prefix made of #!$...

Not sure I follow that ;-)

Sure you'll understand while reading the code (just because os.path.commonsuffix does not exist)

takluyver · 2012-06-13T20:49:03Z

I did understand the code...eventually ;-) I'm pretty sure there should be a more readable solution. It might involve a regex.

minrk · 2012-06-13T21:01:44Z

I'm pretty sure there should be a more readable solution. It might involve a regex.

I think I'm going to keep this quote for a long time.

fperez · 2012-06-13T21:28:35Z

On Wed, Jun 13, 2012 at 2:01 PM, Min RK
reply@reply.github.com
wrote:

I think I'm going to keep this quote for a long time.

+1

takluyver · 2012-06-13T21:31:53Z

Yes, it did feel a bit odd typing that. I stand by it, though - regexes have their place.

Thinking about this a bit more, I'm not sure that common_prefix is the easiest place to fix it - because the easy solution requires knowing the text that's already there. @Carreau , have a look at the cleanup code that's already in _handle_complete_reply - I think that a fix there might be simpler:

https://github.com/ipython/ipython/blob/master/IPython/frontend/qt/console/ipython_widget.py#L138

minrk · 2012-06-13T21:37:07Z

I stand by it, though - regexes have their place.

Absolutely true, and I am a lot more comfortable with them now than I used to be. Still an amusing statement.

Carreau · 2012-06-14T06:34:19Z

@takluyver,

IPython console :

typed >>>  ti<tab>
compl >>>  ti   # I would expect time....
%%timeit  %time     %timeit

IPython qtonsole (myfix):

typed >>>  ti<tab>
compl >>>  %time # got a leading %
%%timeit  %time     %timeit

In[1] >>>  import time
typed >>>  ti<tab>
compl >>>  time 
%%timeit  %time     %timeit   time

At least for me... so IMHO, the current solution is not a the level of what users would expect. It does not transform the prompt to % anymore, but py trying to get a common prefix with what the user typed you can't fast-forward the completion as it does with completions without leading % items.

And I'll let you try with regex :-)

takluyver · 2012-06-14T10:14:35Z

We don't have to achieve everything the user could expect - tab completion is a complex thing, and there will be time to improve it later. What's important is that it shouldn't remove what you've typed without a good reason.

I'll have a play with it later and see what I can come up with.

Carreau · 2012-06-14T11:36:56Z

Sorry, my memory is faulty, I though that at 0.12 milestone, after importing time typing ti<tab> would complete to time and propose %timeit ,%time, and time as available completions, but apparently it was not the case. I just tried to mimic this behaviour as best as I could, because I saw a regression in the current behavior.

So please consider this PR as both a fix and a proposal for enhancement that could eventually be brought to other frontend.

I just want to point out that it is the behaviour of the current notebook completer, but for the sake of consistency between frontend, I would accept to remove tis 'feature' also from the notebook if you think it is necessary.

takluyver · 2012-06-14T11:53:17Z

I don't think consistency between frontends is a big concern right now
for tab completion. So long as each one works and isn't annoying, we
can come back to the system after 0.13.

minrk · 2012-06-15T20:46:35Z

I haven't quite followed this review, so is this one ready for merge now?

Carreau · 2012-06-15T20:51:06Z

@takluyver wrote :

I'll have a play with it later and see what I can come up with.

I think Thomas want go try with regular expressions.

takluyver · 2012-06-15T22:27:33Z

Sorry, I forgot about it. I'll have a look now and see if I can come up with something cleaner.

takluyver · 2012-06-15T23:14:39Z

(Refer to #1972)

takluyver · 2012-06-16T20:33:02Z

Focussing on this one again:

I think if we take the regex from #1972, we can separate prefixes and items more easily:

ESCAPE_RE = re.compile("^[" + ESC_SHELL + ESC_HELP + ESC_MAGIC + ESC_QUOTE + ESC_QUOTE2 + ESC_PAREN + "]+")

first_prefix = ESCAPE_RE.match(min(items)).group(0)
last_prefix = ESCAPE_RE.match(max(items)).group(0)
# ...
items = [ESCAPE_RE.sub("", s) for s in items]

That saves us the think-through-this-carefully lambda to get prefixes. I think the common-suffix line is the best way of doing what it does.

takluyver · 2012-06-16T20:38:43Z

Also, let's make the docstring a bit clearer. Something like:

"Return the longest common prefix of a list of strings, but with special treatment of escape characters that might precede commands in IPython, such as %magic functions. Used in tab completion.

For a more general function, see os.path.commonprefix"

Carreau · 2012-06-17T12:01:21Z

items = [ESCAPE_RE.sub("", s) for s in items]

lstrip is much faster on a few test, i'll keep it

Carreau · 2012-06-17T12:24:58Z

Ok, done with regular expression.
I'm still desapointed that the whole function is not just a big regex...

takluyver · 2012-06-17T13:41:51Z

OK, this is looking OK to me, and I've just done a quick manual test. I'm happy for you to merge this when you're ready.

Carreau · 2012-06-17T14:42:21Z

Merging then.

[qtconsole] take %,%% prefix into account for completion

takluyver reviewed Jun 13, 2012
View reviewed changes

take #!%... prefix into account for completion

3ee91c3

fixes ipython#1767

takluyver mentioned this pull request Jun 15, 2012

Fix completion of '%tim' in the Qt console #1972

Closed

find prefix with regex

0dc36ea

Carreau added a commit that referenced this pull request Jun 17, 2012

Merge pull request #1944 from Carreau/completer-cell-magic

2484ba1

[qtconsole] take %,%% prefix into account for completion

Carreau merged commit 2484ba1 into ipython:master Jun 17, 2012

mattvonrocketstein pushed a commit to mattvonrocketstein/ipython that referenced this pull request Nov 3, 2014

Merge pull request ipython#1944 from Carreau/completer-cell-magic

7455255

[qtconsole] take %,%% prefix into account for completion

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[qtconsole] take %,%% prefix into account for completion #1944

[qtconsole] take %,%% prefix into account for completion #1944

Carreau commented Jun 13, 2012

fperez commented Jun 13, 2012

takluyver commented Jun 13, 2012

fperez commented Jun 13, 2012

takluyver Jun 13, 2012

minrk Jun 13, 2012

takluyver commented Jun 13, 2012

Carreau commented Jun 13, 2012

takluyver commented Jun 13, 2012

Carreau commented Jun 13, 2012

takluyver commented Jun 13, 2012

minrk commented Jun 13, 2012

fperez commented Jun 13, 2012

takluyver commented Jun 13, 2012

minrk commented Jun 13, 2012

Carreau commented Jun 14, 2012

takluyver commented Jun 14, 2012

Carreau commented Jun 14, 2012

takluyver commented Jun 14, 2012

minrk commented Jun 15, 2012

Carreau commented Jun 15, 2012

takluyver commented Jun 15, 2012

takluyver commented Jun 15, 2012

takluyver commented Jun 16, 2012

takluyver commented Jun 16, 2012

Carreau commented Jun 17, 2012

Carreau commented Jun 17, 2012

takluyver commented Jun 17, 2012

Carreau commented Jun 17, 2012

[qtconsole] take %,%% prefix into account for completion #1944

[qtconsole] take %,%% prefix into account for completion #1944

Conversation

Carreau commented Jun 13, 2012

fperez commented Jun 13, 2012

takluyver commented Jun 13, 2012

fperez commented Jun 13, 2012

takluyver Jun 13, 2012

Choose a reason for hiding this comment

minrk Jun 13, 2012

Choose a reason for hiding this comment

takluyver commented Jun 13, 2012

Carreau commented Jun 13, 2012

takluyver commented Jun 13, 2012

Carreau commented Jun 13, 2012

takluyver commented Jun 13, 2012

minrk commented Jun 13, 2012

fperez commented Jun 13, 2012

takluyver commented Jun 13, 2012

minrk commented Jun 13, 2012

Carreau commented Jun 14, 2012

takluyver commented Jun 14, 2012

Carreau commented Jun 14, 2012

takluyver commented Jun 14, 2012

minrk commented Jun 15, 2012

Carreau commented Jun 15, 2012

takluyver commented Jun 15, 2012

takluyver commented Jun 15, 2012

takluyver commented Jun 16, 2012

takluyver commented Jun 16, 2012

Carreau commented Jun 17, 2012

Carreau commented Jun 17, 2012

takluyver commented Jun 17, 2012

Carreau commented Jun 17, 2012