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

Fix #448 #451

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Fix #448 #451

wants to merge 4 commits into from

Conversation

cantino
Copy link
Owner

@cantino cantino commented Jan 25, 2025

This should fix 448, I think, but I'm not sure that it's tuned right. I don't use FUZZY, so perhaps someone who does can tweak it? @unexge

@unexge
Copy link

unexge commented Jan 26, 2025

Thanks @cantino! I can apply this patch and test it for a couple of days to see if the results are good.

@unexge
Copy link

unexge commented Jan 27, 2025

So with the main branch, when I search for get pods I was getting k get pods as first result (which is what I expect), but with this change now it's the second match and first match seems less related (it has pod in it, but still I wouldn't expect it to be first):

main:
Screenshot 2025-01-27 at 11 42 17

this patch:
Screenshot 2025-01-27 at 11 43 30

I'm not very familiar with the codebase, I'll try to understand the sorting logic to see if I can tweak it to have similar results to the previous version.

Also, to overcome the original crash I just replaced sorted_by with sorted_unstable_by so it doesn't require total order and doesn't crash, with that version I think the current logic/sorting was preserved. Maybe that can be an alternative fix, but not sure if it messes up with some other cases.

@cantino
Copy link
Owner Author

cantino commented Jan 29, 2025

Hey @unexge, sorted_unstable_by seems like a much simpler solution. Was that giving you results you're happy with? If so, I'll just do that.

@unexge
Copy link

unexge commented Jan 29, 2025

Hey @cantino, yes, from my (limited) experience it's giving very similar results to the latest released version, which I was happy with.

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.

2 participants