-
Notifications
You must be signed in to change notification settings - Fork 681
[feat] Rework reconnect logic to actually create a new connection instead of only changing the database (#746) #1416
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
[feat] Rework reconnect logic to actually create a new connection instead of only changing the database (#746) #1416
Conversation
… for use by the command \r to call the new reconnect function. Updated help output in tests to match the change.
… for use by the command \r to call the new reconnect function. Updated help output in tests to match the change.
…emes/mycli into feat/746/rework-reconnect-command
| special.register_special_command(self.change_db, "use", "\\u", "Change to a new database.", aliases=["\\u"]) | ||
| special.register_special_command( | ||
| self.change_db, | ||
| "connect", |
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.
Changing the full command from "connect" to "reconnect" is a needless breaking change for the user. Maybe it is more rational since both the long and short versions start with "r"? We should be able to add "reconnect" as an alias here:
aliases=["\\r", "reconnect"],
if that is the motivation.
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.
Good point! I switched it back to connect. Goal was consistency, but your point makes sense and it matches the official client anyway
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.
And RE: adding the alias, I pondered it but decided to leave it off for now. Theoretically in some future change we could split it out to actually be "connect" and "reconnect" with different logic (may never happen, but possible) so didn't want to further muddy the waters there.
rolandwalker
left a comment
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.
Nice.
Description
What:
Reworks the reconnect logic (both the command
\rand calls from other parts of the code) to actually create a new database connection instead of simply running a command to change the database.Why:
With the existing reconnect logic, some people would get long reconnect times. Plus the logic to reconnect was more indirect as it was a result of calling another command instead of specifically creating a new connection.
Resolves #746
Checklist
changelog.md.AUTHORSfile (or it's already there).uv run ruff check && uv run ruff format && uv run mypy --install-types .to lint and format the code.