Skip to content

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.

@cantino cantino mentioned this pull request Feb 2, 2025
@cantino
Copy link
Owner Author

cantino commented Feb 2, 2025

Okay cool, I'll merge #454 instead.

@cantino cantino closed this Feb 2, 2025
@cantino cantino deleted the fix-448 branch February 2, 2025 04:28
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