-
-
Notifications
You must be signed in to change notification settings - Fork 543
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
Add SearchMode fzf. #279
Add SearchMode fzf. #279
Conversation
Add a new search mode "fzf" that tries to mimic the search syntax of https://github.com/junegunn/fzf#search-syntax This search mode splits the query into terms where each term is matched individually. Terms can have operators like prefix, suffix, exact match only and can be inverted. Additionally, smart-case matching is performed: if a term contains a non-lowercase letter the match will be case-sensitive.
You can use |
Thanks for this! 🥳 I'll give the code a proper look later, but will try running your branch for a bit today. |
I'm aware that this is possible. But implementing the search mode at query time allows us to use the other filters like |
IMO, it's a bad idea to implement such features instead of improving interop with external software, as it increases maintenance, binary size and there will be implementation differences. Additionally having a better pipe support would benefit not only |
I do also think that this extra complexity needs to be taken carefully. I have some (long overdue) clean ups of some of the sql code to remove the use of However, if this is a feature that's very desirable, then it does make sense to take on said complexity |
I think I'm happy for us to take on the complexity here. I'd suggest that we actually replace the current fuzzy implementation with this though, rather than adding a new setting called This way we're not tied to the implication we will match fzf, and can expand with more improvements in the future if needs be I appreciate the concern there @panekj, but I'd rather focus on the use case of users using our TUI. Searching and then piping works fine, but it's not as good (and probably never can be as good) as searching with the correct term in the first place |
- Use SearchMode::Fuzzy instead of SearchMode::Fzf - update docs - re-order tests so previous fuzzy tests come first, add more tests for each operator
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, code looks good. Just some nitpicks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Thanks @pmarschik! Can't wait to get this released 😁 🚀 |
f861893 Update to clap 3.1.x (#289) e8f7aac Add compact mode (#288) 1e04c4c Add rust-version to Cargo.toml (#287) 222e52b Update Dockerfile fae118a Improve fuzzy search (#279) 7cde55a Add code of conduct (#281) d270798 Update config-rs (#280) 3248883 Update README.md 7f58741 Fix `history list --cwd` errors (#278) e117b62 Update fish bindings. (#265) 4223ac6 Restore bash 4.2 compatibility, only add hook once (#271) 7651f89 Add support for blesh (#267) c2dd332 fix: get install.sh working on UbuntuWSL (#260) 84403a3 Bump reqwest from 0.11.7 to 0.11.9 (#261) 5005cf7 Bump serde_json from 1.0.73 to 1.0.75 (#262) 7fa3e1c Do not crash if the history timestamp is in the future (#250) 8d21506 use sqlite grouping rather than subquery (#181) d36ff13 Replace dpkg with apt (#248)
f861893 Update to clap 3.1.x (#289) e8f7aac Add compact mode (#288) 1e04c4c Add rust-version to Cargo.toml (#287) 222e52b Update Dockerfile fae118a Improve fuzzy search (#279) 7cde55a Add code of conduct (#281) d270798 Update config-rs (#280) 3248883 Update README.md 7f58741 Fix `history list --cwd` errors (#278) e117b62 Update fish bindings. (#265) 4223ac6 Restore bash 4.2 compatibility, only add hook once (#271) 7651f89 Add support for blesh (#267) c2dd332 fix: get install.sh working on UbuntuWSL (#260) 84403a3 Bump reqwest from 0.11.7 to 0.11.9 (#261) 5005cf7 Bump serde_json from 1.0.73 to 1.0.75 (#262) 7fa3e1c Do not crash if the history timestamp is in the future (#250) 8d21506 use sqlite grouping rather than subquery (#181) d36ff13 Replace dpkg with apt (#248)
Add a new search mode "fzf" that tries to mimic the search syntax of
https://github.com/junegunn/fzf#search-syntax
This search mode splits the query into terms where each term is matched
individually. Terms can have operators like prefix, suffix, exact match
only and can be inverted. Additionally, smart-case matching is
performed: if a term contains a non-lowercase letter the match will be
case-sensitive.
I've also refactored the client database tests to be more readable.
Future improvements: