Skip to content
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

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

lazmond3
Copy link

@lazmond3 lazmond3 commented Aug 3, 2024

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. Using FormattedText from prompt_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 default length option is used, searching for a common term like select would bring up entries where select 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 a toolkit subpackage within the packages 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 the prompt_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

  • I've added this contribution to the changelog.md.
  • I've added my name to the AUTHORS file (or it's already there).

@amjith
Copy link
Member

amjith commented Aug 6, 2024

Sorry about the delay in response. I'll give it shot later today and provide feedback. Thank you for taking the time to contribute.

self.filename = filename
super().__init__(filename)

def load_history_strings(self) -> Iterable[str]:
Copy link
Author

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.

Copy link
Author

@lazmond3 lazmond3 left a 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.
Copy link
Author

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.

@lazmond3
Copy link
Author

lazmond3 commented Aug 8, 2024

@amjith

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:

RuntimeError: Support for the test command was removed in Setuptools 72

To resolve this issue, I propose modifying the command in the CI step as follows:

- name: Pytest / behave
  env:
    PYTEST_PASSWORD: root
    PYTEST_HOST: 127.0.0.1
  run: |
    pytest test --cov-report --cov=mycli 

Would it be acceptable for me to make this change to ensure the tests run successfully?

@rolandwalker
Copy link
Contributor

What happens if you change the line setuptools in requirements-dev.txt to setuptools<=71.1.0 ?

@amjith
Copy link
Member

amjith commented Sep 9, 2024

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.

@amjith
Copy link
Member

amjith commented Sep 9, 2024

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.

@lazmond3
Copy link
Author

@amjith @rolandwalker

Thank you for your guidance. Following @rolandwalker 's suggestion to specify setuptools <= 71.1.0 in the CI configuration has proven successful. Considering that the required changes are minimal, I intend to proceed with this approach to ensure that the CI passes. I appreciate your assistance and look forward to your feedback.

@lazmond3
Copy link
Author

lazmond3 commented Oct 4, 2024

I am testing with the forked repository, but it seems that the CI failures are not due to code changes.
They are likely caused by the impact of changes in library versions and such. I am currently investigating.

@lazmond3
Copy link
Author

lazmond3 commented Oct 5, 2024

@amjith
Thank you for your patience. We have identified a solution to address the CI failure.

Cause of CI Failure

CI Link: GitHub Actions

In this CI, the following two steps are experiencing failures:

  • test/features/connection.feature:4: Execute mycli on localhost without specifying a port
  • test/features/connection.feature:20: Execute mycli without specifying a host and port

These tests fail when mycli attempts to connect to the MySQL server using a socket file. The error message encountered is as follows:

error('unpack requires a buffer of 4 bytes')

Problem Investigation

The 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 /var/run/mysqld/mysqlx.sock, which is intended for a different protocol X server, results in the error "unpack requires a buffer of 4 bytes." In other words, it seems that the root cause of error #1146 might have been an attempt to connect to the MySQL server using the socket file intended for the X plugin, possibly due to incorrect socket file guess results.

Fix Details

The issue can be resolved by filtering out mysqlx.sock in guess_socket_location. I pushed this fix to the following PR in the repository I forked, and confirmed that the CI passed.

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 setuptools against the main branch, yet it failed with the same error as mentioned above.


In PR #1170, I intend to include the fix for guess_socket_location to address the CI issue and "Close #1146" simultaneously. Is that acceptable?

changelog.md Outdated Show resolved Hide resolved
@lazmond3
Copy link
Author

@amjith @rolandwalker

I implemented the solution I mentioned the other day, fixed the failing CI, and simultaneously resolved issue #1146.

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.

3 participants