Skip to content
New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[notebook] deduplicate completion results #1841

Merged
merged 4 commits into from Jun 8, 2012

Conversation

Carreau
Copy link
Member

@Carreau Carreau commented Jun 3, 2012

remove context completion that are duplicates from introspection
completion

fixes #1840

remove context completion that are duplicates from introspection
completion
fixes ipython#1840
@Carreau
Copy link
Member Author

Carreau commented Jun 3, 2012

made a mistake, close, will reopen.

@Carreau Carreau closed this Jun 3, 2012
@Carreau Carreau reopened this Jun 3, 2012
@Carreau
Copy link
Member Author

Carreau commented Jun 3, 2012

@fperez , this should remove the "duplicates" completions by favoring introspection completion from context completion.

There might (really not sure) be some false positive of 'duplicates', so some context completion might not show up in rare case, but no completion coming from live object introspection will be discarded.

@fperez
Copy link
Member

fperez commented Jun 4, 2012

The functionality seems to work correctly, thanks! I only have a couple of comments:

  1. You are using code in this style of brace handling:
    function _existing_completion(item, completion_array)
    {
        if(item.substr(0,1) == '.')
        {
            for( var c in completion_array )
            {

but in general I see our JS files use the more traditional placing of the opening brace in the same line:

    function _existing_completion(item, completion_array) {
        if(item.substr(0,1) == '.') {
            for( var c in completion_array ) {

so it should probably conform to that.

  1. I don't know enough JS to say anything about the actual implementation of the filtering. It looks a bit complicated to me, in Python I would have simply done right before returning something like:
return sorted(set(matches))

thus ensuring uniqueness, rather than doing all the per-item check you are doing. But again, I don't know much JS, so perhaps there's no cleaner way to do it. @ellisonbg, @minrk, thoughts on this one?

@minrk
Copy link
Member

minrk commented Jun 4, 2012

There doesn't seem to be something like set in javascript, but a quick Googling reveals a pretty simple sort/unique transform, that could probably be even simpler if you know that everything is a string:

http://stackoverflow.com/questions/4833651/javascript-array-sort-and-unique

@Carreau
Copy link
Member Author

Carreau commented Jun 4, 2012

Well, issue is that both introspection and context completion does not return the same thing.
If you try to complete a.b.<tab> kernel will return a.b.something whereas coompletion based on codemirror token will only return .something , so as the 2 kinds of completion are not the same length you have to keep the replace range tightly linked to the completion, then i'm sorting and comparing dict based on their keys.

And as the 2 keys are different I have the deduplication try to match only the ends of the strings, but only when one of them start with a dot, because obviously, you don't want to find that width is the same as set_width.

Still I'll see If I can shorten the logic.

As for the brace, i'll never get used to it, will fix it.

@fperez
Copy link
Member

fperez commented Jun 5, 2012

Great, thanks. Ping us when you've updated the logic and we'll revisit, I'd really like to see this merged.

@Carreau
Copy link
Member Author

Carreau commented Jun 5, 2012

Well, I tried, there is one check that I can omit, but I don't see how I can deduplicate faster. Sorting does not help, uniqueness is not well enough related to comparing items.

assuming I want methAfrom object object. Then object.<tab> gives me :

  • object.methA # from kernel
  • object.methB # from kernel
  • .methA # from codemirror.
  • .methB # from codemirror.

Which is already sorted. and obviously the duplicates are not next to each other... so you can't test for equality, or use hash, set ....

If you have any more ideas ... or i'm good to go

@ellisonbg
Copy link
Member

I think this can go in as is.

@fperez
Copy link
Member

fperez commented Jun 8, 2012

I'm OK too if there's no better option, I just wanted to look into it. Thanks for taking the time to investigate, @Carreau. I'm merging now b/c this bug is really annoying me.

fperez added a commit that referenced this pull request Jun 8, 2012
[notebook] deduplicate completion results

remove context completions that are duplicates from introspection
completion

fixes #1840
@fperez fperez merged commit 09ae0f3 into ipython:master Jun 8, 2012
mattvonrocketstein pushed a commit to mattvonrocketstein/ipython that referenced this pull request Nov 3, 2014
[notebook] deduplicate completion results

remove context completions that are duplicates from introspection
completion

fixes ipython#1840
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

New JS completer should merge completions before display
4 participants