8000 feat: allow user to Select MatchDirection in Menu by divanshu-go · Pull Request #905 · nushell/reedline · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

feat: allow user to Select MatchDirection in Menu #905

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

feat: allow user to Select MatchDirection in Menu #905

wants to merge 1 commit into from

Conversation

divanshu-go
Copy link
Contributor
  • Added a new MatchDirection enum to specify the direction for searching matches in suggestions. (src/menu/mod.rs)

@divanshu-go
feat: allow user to Select MatchDirection in Menu
ffec8e6 
Signed-off-by: divanshu-go <divanshugrover2009@gmail.com>
@divanshu-go divanshu-go mentioned this pull request Apr 15, 2025
Closed
@ysthakur
Copy link
Member

Thanks for trying to improve Reedline. I'm not sure this is the right way to go, since searching from the right doesn't fully fix the issue. Suppose you type in apply and you have a suggestion for a file named apply-apply.rs. If you search from the right, the second apply would be highlighted rather than the first, which seems unintuitive.

I looked at the conversation on the linked PR, and this comment seems pretty sensible to me: antinomyhq/forge#650 (comment)

How about we create our own CompletionMenu and use that instead of the default one? This way we won't need to wait for the PR to be merged in nushell.

The menus don't get updated too often, so I don't believe it would be a huge maintenance burden. And it would only be temporary, because #798 is on the way. If #798 is merged, it will require Forge to make changes downstream to take advantage of the new match_indices field, but once you do that, you won't need to worry about inaccurate highlighting.

To clarify, I'm not shooting down this PR, I'm still open to merging it, but I'd like to avoid adding a new option to our menus. If #798 gets merged, the match direction stuff would just end up being removed again (even if #798 isn't merged, we'll probably end up making a similar change making the match direction stuff obsolete).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@divanshu-go @ysthakur
0