-
Notifications
You must be signed in to change notification settings - Fork 662
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 fzf history search feature #1170
base: main
Are you sure you want to change the base?
Conversation
Sorry about the delay in response. I'll give it shot later today and provide feedback. Thank you for taking the time to contribute. |
mycli/packages/toolkit/history.py
Outdated
self.filename = filename | ||
super().__init__(filename) | ||
|
||
def load_history_strings(self) -> Iterable[str]: |
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.
This method is same as the superclass, so I can remove this later.
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.
It seems that the pytest for python 3.12 failed. I'll check it later.
changelog.md
Outdated
@@ -23,7 +23,7 @@ Bug Fixes: | |||
|
|||
1.27.1 (2024/03/28) | |||
=================== | |||
|
|||
* Added fzf-like history search functionality. The feature can switch between the old implementation and the new one based on the presence of the fzf binary. |
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.
Not fzf-like, but fzf itself.
It appears that the failure in the CI tests is unrelated to the changes introduced in this PR. The error message indicates that the use of setuptools for running pytest is deprecated and no longer supported:
To resolve this issue, I propose modifying the command in the CI step as follows:
Would it be acceptable for me to make this change to ensure the tests run successfully? |
What happens if you change the line |
This is nice. I have not used fzf before (fishshell user so never really had a need for fzf). I haven't had a chance to review the implementation itself. But wanted to chime in and say nicely done. I like the fact that it uses fzf if it is installed otherwise falls back to the original behavior. I think that is the right approach. I don't want to hide this behind a config option. |
Feel free to modify the CI steps to make the tests pass. I'm planning on modernizing the codebase to switch to pyproject.toml, ruff and uv. So don't worry too much about making it perfect. Just make whatever edits necessary to get the CI to pass. |
Thank you for your guidance. Following @rolandwalker 's suggestion to specify |
I am testing with the forked repository, but it seems that the CI failures are not due to code changes. |
@amjith Cause of CI FailureCI Link: GitHub Actions In this CI, the following two steps are experiencing failures:
These tests fail when mycli attempts to connect to the MySQL server using a socket file. The error message encountered is as follows:
Problem InvestigationThe following issue provided insights that were instrumental in resolving the problem: It has been reported that on Ubuntu 22.04, connections via socket files fail, resulting in the error message "unpack requires a buffer of 4 bytes." Upon examining the behavior of mycli in an Ubuntu 22.04 environment using Docker on a local machine, it was confirmed that the error occurs only when the socket file is not explicitly specified. This causes mycli to connect to the MySQL server via the socket file. The logic used when the socket file is "not explicitly specified" is guess_socket_location, which often returns "mysqlx.sock" before "mysqld.sock" depending on conditions. Additionally, experiments using mycli within the Docker environment revealed that attempting to specify the socket file Fix DetailsThe issue can be resolved by filtering out Additionally, I want to clarify that the CI failure is not caused by my code changes, as shown in the test CI fails by lazmond3 · Pull Request #5 · lazmond3/mycli. In this PR, I only fixed the version of In PR #1170, I intend to include the fix for |
I implemented the solution I mentioned the other day, fixed the failing CI, and simultaneously resolved issue #1146. |
Description
Background
Previously, the history function of mycli only allowed viewing history one line at a time, making it feel like peering through a keyhole. Users had to navigate through the history by typing on the keyboard to see the previous and next entries, making it difficult to find the desired history entry. A request for a feature to enable fzf-like history search was made at #726.
Achievements
Implemented the ability to perform fzf search on mycli history. fzf is a software written in Go, and its binary file is also named fzf. (See: junegunn/fzf: 🌸 A command-line fuzzy finder)
The mycli history fzf search feature is designed to switch between the old implementation and the new one based on the presence of the fzf binary. (It might be better to have this feature turned off by default and enabled via an option. Feedback is welcome.)
The history format is
<timestamp> <stmt>
. (Placing the timestamp at the end makes formatting difficult and searching by date harder.) Currently, the timestamp is handled as a string and displayed with seconds. UsingFormattedText
fromprompt_toolkit
to gray out the timestamp could improve visibility, but to keep the scope of this PR focused, this enhancement is not included.Since fzf provides line-based selection functionality, newlines in the history are replaced with spaces for the suggestion list. However, when setting data to the buffer, the original text is retrieved and set.
Regarding the fzf search score, the
tiebreak=index
option is used. This is because I believe that when searching for SQL queries, users often prefer to use the most recent ones. If the defaultlength
option is used, searching for a common term likeselect
would bring up entries whereselect
was mistakenly entered by itself, which can be annoying. For more details, see the fzf tiebreak option documentation.This implementation is an ad-hoc override of some
prompt_toolkit
behavior. Therefore, the implementation is gathered under atoolkit
subpackage within thepackages
directory.Alternative Approach
Ideally, we should contribute to
prompt_toolkit
and use the history functionality implemented there in mycli. However,prompt_toolkit
is pure Python, and fzf cannot be integrated directly. Rewriting the Go-based program in Python and adapting it to theprompt_toolkit
Completion mechanism would be a significant undertaking.For this reason, I decided to override some history-related implementations and keybindings in mycli.
Comments
mycli is a great tool, and I have greatly benefited from it. I am pleased to have the opportunity to contribute to this excellent software and hope that mycli will become an even more convenient tool.
Checklist
changelog.md
.AUTHORS
file (or it's already there).