Skip to content

fix: accurately locate the match within the suggestion value #903

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

Merged
merged 7 commits into from
Apr 15, 2025

Conversation

divanshu-go
Copy link
Contributor

before

image

after

image

Signed-off-by: divanshu-go <divanshugrover2009@gmail.com>
@blindFS
Copy link
Contributor

blindFS commented Apr 10, 2025

There's this long existing PR #798 😅, which handles fuzzy matching as well.

@divanshu-go
Copy link
Contributor Author

@blindFS What can I do that in case ?

Also, Is anything wrong with this PR's code ?

@blindFS
Copy link
Contributor

blindFS commented Apr 10, 2025

@blindFS What can I do that in case ?

Honestly, IDK, I think there's a refactoring of that PR going on, @ysthakur.

Also, Is anything wrong with this PR's code ?

I'm not sure how this looks for fuzzy matched texts. And you probably need to add the same fix to ide_menu as well.

Oh, we were planning to add an optional field for matched-span info to Suggestion. That will delegate such problem to downstream programs to solve. No guarantee of the delivery date of that, if you're up-to add that feature, I think it would be very helpful.

Signed-off-by: divanshu-go <divanshugrover2009@gmail.com>
@divanshu-go
Copy link
Contributor Author

@blindFS
I did changes in ide_menu.rs

@divanshu-go
Copy link
Contributor Author

@blindFS This PR is opened in favour to handle this issue antinomyhq/forge#420

The previous implementation in columnar_menu.rs of func create_string assumed that it will only found the match from start of the string

so instead of this I edited that func create_string to break the string in three parts (prefix, matched_part, remaining_str)
this approach solved the issue for us

Signed-off-by: divanshu-go <divanshugrover2009@gmail.com>
@fdncred
Copy link
Contributor

fdncred commented Apr 10, 2025

Please excuse my ignorance, but what does the @ signify? I assume you're doing @apply<tab> in the example above. Also, what folder are you in when you're trying to complete? or maybe it's a history completion and not a path completion?

@divanshu-go
Copy link
Contributor Author

Please excuse my ignorance, but what does the @ signify?

in our cli (which is a shell) , when the user press @, then he is able to select a file

I assume you're doing @apply<tab> in the example above.

yes , and then it must search all the files having name apply

Also, what folder are you in when you're trying to complete?

I was in a folder named forge

or maybe it's a history completion and not a path completion?

its clearly a path completion

Signed-off-by: divanshu-go <divanshugrover2009@gmail.com>
Copy link
Member

@ysthakur ysthakur left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on this! As blindFS mentioned, #798 is still being worked on, so this will be good to have in the meantime (I do plan to update #798 soon™). It's fine that this doesn't handle fuzzily matched suggestions, since it's better than what we have now.

I'd recommend testing out your changes with descriptions added to the suggestions (you can edit the DefaultCompleter to add a dummy description to each Suggestion or something like that).

Signed-off-by: divanshu-go <divanshugrover2009@gmail.com>
Signed-off-by: divanshu-go <divanshugrover2009@gmail.com>
@divanshu-go divanshu-go requested a review from ysthakur April 11, 2025 08:58
Signed-off-by: divanshu-go <divanshugrover2009@gmail.com>
@divanshu-go
Copy link
Contributor Author

@ysthakur I have did the code changes required by You
Are there any further requirements for this PR ?

@ysthakur
Copy link
Member

Yup I saw you updated the PR, code looks good to me but I haven't gotten around to running it on my computer again. Give me a day or two

Copy link
Member

@ysthakur ysthakur left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just tested the columnar menu and it works. Thanks for your work on this.

@ysthakur ysthakur merged commit 2dba3d3 into nushell:main Apr 15, 2025
6 checks passed
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.

4 participants