Skip to content

Don't emit keyring-saved message unless needed#1510

Merged
rolandwalker merged 1 commit intomainfrom
RW/avoid-keyring-saved-message
Feb 5, 2026
Merged

Don't emit keyring-saved message unless needed#1510
rolandwalker merged 1 commit intomainfrom
RW/avoid-keyring-saved-message

Conversation

@rolandwalker
Copy link
Contributor

Description

Don't emit keyring-saved message unless needed

Incidentally update message wordings to match CLI option: "keyring" rather than "keychain".

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 .

@rolandwalker rolandwalker self-assigned this Feb 4, 2026
Copy link
Contributor

@scottnemes scottnemes left a comment

Choose a reason for hiding this comment

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

Works as-is.

Question for this or a future PR though:

Should we be waiting until after a successful connection to save the password to the keyring? As-is it will save and then go on to fail the connection if the password is wrong. I don't normally use a keyring so not sure what behavior the user would expect, but seems like it'd be preferable to only save if the password works.

Incidentally update message wordings to match CLI option: "keyring"
rather than "keychain".
@rolandwalker rolandwalker force-pushed the RW/avoid-keyring-saved-message branch from b7068cc to 190788c Compare February 5, 2026 08:47
@rolandwalker
Copy link
Contributor Author

Great point for future work: save the password only on a successful connection.

@rolandwalker rolandwalker merged commit 90ca223 into main Feb 5, 2026
8 checks passed
@rolandwalker rolandwalker deleted the RW/avoid-keyring-saved-message branch February 5, 2026 09:06
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