Skip to content

Fix drivers completions#481

Closed
remisalmon wants to merge 1 commit intoxo:masterfrom
remisalmon:fix-drivers-completions
Closed

Fix drivers completions#481
remisalmon wants to merge 1 commit intoxo:masterfrom
remisalmon:fix-drivers-completions

Conversation

@remisalmon
Copy link
Contributor

Fixes #478

The drivers NewCompleter was removed from the handler between v0.18.1 and v0.19.0.
Tested locally, I had to cleanup the dependencies for go to build.

Copy link
Member

@nineinchnick nineinchnick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for working on it! I found that the completer calls were moved from Open to New in 05236d1. We should figure out why, to avoid calling it twice. @kenshaw do you have more context why? Should we revert this one change?

@remisalmon
Copy link
Contributor Author

Thanks a lot for working on it! I found that the completer calls were moved from Open to New in 05236d1. We should figure out why, to avoid calling it twice. @kenshaw do you have more context why? Should we revert this one change?

I was thinking to also move the drivers.NewCompleter() call to New but New is missing the context that this function needs.
I moved all completer calls back to Open in 2ad11cf to simplify a little but I can revert if you prefer?

@nestor-custodio
Copy link

At the risk of coming across like a nag, I think bumping this PR seems warranted given that the issue persists and we've seen 15 releases since the proposed fix was submitted for approval. 😶‍🌫

@stephane-klein
Copy link

bump

@remisalmon
Copy link
Contributor Author

I just tested this patch with the latest master and it still works on my end. Asking for a re review 🤞

if err = drivers.Ping(ctx, h.u, h.db); err == nil {
if h.l.Interactive() {
h.l.SetOutput(h.outputHighlighter)
h.l.Completer(completer.NewDefaultCompleter(completer.WithConnStrings(h.connStrings())))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks redundant. We should leave this in New, and only create the driver's completer here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reverted that in 14d1f30.

@kenshaw
Copy link
Member

kenshaw commented Jan 6, 2026

@remisalmon Hi, I apologize for not looking at this earlier. Could you squash the commits into a single commit and I'll rebase onto master and backport it to the release-20 branch? Thanks.

@remisalmon remisalmon force-pushed the fix-drivers-completions branch from 14d1f30 to 79c6e9c Compare January 7, 2026 00:38
@kenshaw kenshaw closed this Jan 7, 2026
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.

table name tab-completion not working between 0.18.x -> 0.19.x

5 participants