dbcli / mycli

A Terminal Client for MySQL with AutoCompletion and Syntax Highlighting.
http://mycli.net
Other
11.32k stars 656 forks source link

Add better sorting of completions #1165

Open schmeic opened 3 weeks ago

schmeic commented 3 weeks ago

Description

Sort completions by match point, then match length, then item text.

Note that match length is equal to the length of the search text for non fuzzy matches, and equal to the length of the match group for fuzzy matches.

This means that matches that start with the search text will be displayed first, followed by matches that contain the search text, and then fuzzy matches.

Sorting by item text when both match point and match length are the same for matched completions ensures that completions that start with the search text will be in alphabetical order. For example, searching for fr will display FROM before _FROMUNIXTIME.

Checklist

rolandwalker commented 3 weeks ago

Wow, you weren't joking about the tests.

From the English description, the algorithm sounds great.

I may have time to experiment with this on the weekend.

schmeic commented 3 weeks ago

Yeah, that's why I wanted to make sure the algorithm would be accepted before working on the tests. From my experiments with this new sorting, I've found that it has always put what I consider the most relevant results at the top.

The funny thing was that the find_matches(...) function was already storing all 3 of the parameters I used for sorting, and then ignoring everything but the item text at the end when creating the completions generator. That's what gave me the idea for the sorting algorithm.

rolandwalker commented 3 weeks ago

Let's let the PR hang around for some days and see if it attracts any objections.

schmeic commented 3 weeks ago

Ok. Maybe you'll have some time to experiment with it as well. I'm hoping it will attract some "likes"...

rolandwalker commented 3 weeks ago

Should the proposed change only apply when if not smart_completion: ? It would see that the point of smart_completion is to give categories in category order.

schmeic commented 3 weeks ago

I don't think so, I personally would rather have a completion at the top that best matches my search text regardless of the category. Have to tried it out yet? Maybe you will agree after some experimenting.

I actually first tried doing the same sort in the find_matches(...) function, so per category, and the results were not ideal.

rolandwalker commented 3 weeks ago

I'm trying it now. It would seem that the config file lies when it claims

# Enables context sensitive auto-completion. If this is disabled then all
# possible completions will be listed.
smart_completion = True

because when set to False, table names and such are simply not returned.

It is rather helpful to see the completions in the "smart" ordering when nothing has yet been typed, like maybe

            if len(word_before_cursor) != 0 and word_before_cursor[-1] != '(':
                matches = sorted(matches, key=lambda m: (m[1], m[0], m[2].lower().strip('`')))
schmeic commented 3 weeks ago

Regarding the smart_completion = False setting, I think what you're seeing is a bug. You need to first do:

use <db name>

and then you should see table and column names. It doesn't work if you use a DSN to select the db.

By the way, I think it would be really useful to do fuzzy matching when not doing smart completion. Maybe this can be implemented as a setting?

rolandwalker commented 3 weeks ago

Why closed?

schmeic commented 3 weeks ago

Sorry, didn't realize I did that. I deleted a comment and maybe that closed it?

How can I reopen?

schmeic commented 3 weeks ago

Accidentally closed.

rolandwalker commented 2 weeks ago

Regarding the smart_completion = False setting, I think what you're seeing is a bug. You need to first do: use and then you should see table and column names. It doesn't work if you use a DSN to select the db.

Your suggestion works! But we really need to fix that bug.

By the way, I think it would be really useful to do fuzzy matching when not doing smart completion. Maybe this can be implemented as a setting?

Yes, though one issue is that I adore settings, and @amjith really does not. For example for the entire functionality in this PR, I think it would be fantastic if ~/.myclirc added

completion_aggresive_sort = True

or any other property name that makes sense.

I personally would rather have a completion at the top that best matches my search text regardless of the category.

Right. I personally would also prefer to use your algorithm. But we have to think of the most general usecase and the default expectation. That is why I proposed to enable it outside of the smart_completion codepath.

amjith commented 2 weeks ago

Let me catch up on the PR.

I'm not opposed to adding a config option but if the new completion behavior feels logical we can switch to it by default. Let me read through the proposal and try it out locally.

schmeic commented 2 weeks ago

I think this new sort is much better than the current default, so it seems like most people would prefer it. By the way, I just upgraded to the latest pip version of 1.27.2, and the sorting is now much worse than before. Now it appears to be returning results in alphabetical order, where as before the order was more by relevance (previous version I used was 1.26.1).

So now in 1.27.2, when I do this: SELECT * FROM e I see a bunch of tables starting with a at the top of the results. Previously, the top results were tables starting with e.

Also, there are other open issues I've seen where people complain about the fuzzy matching and want an option to turn it off because the fuzzy matches are often at the top of the results. My algorithm will also fix this since the fuzzy matches will always be below exact matches.

amjith commented 2 weeks ago

I'm still testing things out. Notes so far, I like that FROM is listed about FROM_UNIXTIME. There is a crash that happens when I type SOURCE. Haven't dug into it yet, if you have the time please take a look.

schmeic commented 2 weeks ago

I'm still testing things out. Notes so far, I like that FROM is listed about FROM_UNIXTIME. There is a crash that happens when I type SOURCE. Haven't dug into it yet, if you have the time please take a look.

Ok, I see the issue. The find_files() method is returning a Completion generator and the code is expecting an array of match tuples.

Any idea what happened to the sorting in the latest version (1.27.1) ? Why is it now alphabetical?

schmeic commented 1 week ago

Still waiting on feedback for this one...

amjith commented 1 week ago

I've tested it locally and I'm fine with the ordering. Happy to merge if you can fix the tests.

schmeic commented 1 week ago

Cool, I'll work on fixing the tests.