Skip to content

Break sqlcompleter.py find_matches() into units, and add test coverage.#1754

Open
rolandwalker wants to merge 1 commit intomainfrom
RW/break-down-find-matches
Open

Break sqlcompleter.py find_matches() into units, and add test coverage.#1754
rolandwalker wants to merge 1 commit intomainfrom
RW/break-down-find-matches

Conversation

@rolandwalker
Copy link
Copy Markdown
Contributor

Description

This also changes find_matches() into an instance method, but we could consider changing find_matches() and many others into static methods.

Motivation: smaller units make the code more testable and more amenable to agentic coding.

Checklist

  • I added this contribution to the changelog.md file.
  • I added my name to the AUTHORS file (or it's already there).
  • To lint and format the code, I ran
    uv run ruff check && uv run ruff format && uv run mypy --install-types .

and add test coverage.

This also changes find_matches() into an instance method, but we could
consider changing find_matches() and many others into static methods.

Motivation: smaller units make the code more testable and more
amenable to agentic coding.
@rolandwalker rolandwalker self-assigned this Mar 28, 2026
@github-actions
Copy link
Copy Markdown

  1. Potential compatibility regression: find_matches is no longer callable as a class/static method
    In this PR, find_matches changed from @staticmethod to an instance method (self first arg) at mycli/sqlcompleter.py:1304.
    Any external usage like SQLCompleter.find_matches("sel", collection) now raises TypeError instead of working as before.
    Action: either restore static compatibility (keep find_matches static and refactor helpers accordingly), or explicitly document this as a breaking API change and add a regression test asserting intended calling style.

  2. Missing test for the above API behavior
    New tests cover internals heavily, but none verify whether class-level invocation remains supported after refactor at test/pytests/test_sqlcompleter_find_matches.py.
    Action: add one test for SQLCompleter.find_matches(...) behavior (supported or intentionally unsupported) to lock in expected API behavior.

I didn’t find a new security issue in this diff.
I could not run the tests in this environment because uv is unavailable.

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.

2 participants