Skip to content

Comments

Liberally accept literal DSN arguments to --dsn#1589

Merged
rolandwalker merged 1 commit intomainfrom
RW/accept-literal-dsns-along-with-aliases
Feb 20, 2026
Merged

Liberally accept literal DSN arguments to --dsn#1589
rolandwalker merged 1 commit intomainfrom
RW/accept-literal-dsns-along-with-aliases

Conversation

@rolandwalker
Copy link
Contributor

Description

Forgive the user for thinking that --dsn can accept a literal DSN in addition to an alias.

Internally, recast the confusing variable name dsn to dsn_alias.

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 20, 2026
@github-actions
Copy link

Findings

  1. Missing test coverage for new --dsn literal behavior
    The PR adds support for passing a literal DSN via --dsn, but test/test_main.py only covers --dsn with aliases. Add a test that invokes --dsn mysql://... and asserts the parsed connection args (and that CLI flags override DSN fields, mirroring existing DSN-via-positional tests). This guards the new behavior from regressions.
    File: test/test_main.py

No security issues or correctness regressions jumped out in the code paths touched.

Suggested next steps

  1. Add a test in test/test_main.py for --dsn with a literal DSN, including a variant where --user/--password/--host/--port/--database override fields from the DSN.

@rolandwalker rolandwalker force-pushed the RW/accept-literal-dsns-along-with-aliases branch 2 times, most recently from 9505a68 to 1774657 Compare February 20, 2026 12:39
mycli/main.py Outdated
@click.option("--list-dsn", "list_dsn", is_flag=True, help="list of DSN configured into the [alias_dsn] section of myclirc file.")
@click.option("-D", "--database", "dbname", help="Database or DSN to use for the connection.")
@click.option("-d", "--dsn", 'dsn_alias', default="", envvar="DSN", help="DSN alias configured in the ~/.myclirc file, or a full DSN.")
@click.option("--list-dsn", "list_dsn", is_flag=True, help="list of DSNs configured in the [alias_dsn] section of the ~/.myclirc file.")
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe change the help to "list of DSN aliases..." to fit with your rename

Copy link
Contributor

Choose a reason for hiding this comment

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

Also maybe add an alias command for --list-dsn-aliases or something again to fit with your rename.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also maybe add a --dsn-alias alias as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point on the help text, will do!

But as to the other two, it isn't a user-visible rename, just a fallback, with an internal variable rename for clarity.

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.

Verified it works as-is. Left some comments for further potential aliases if desired.

@rolandwalker rolandwalker force-pushed the RW/accept-literal-dsns-along-with-aliases branch from 1774657 to 77bc723 Compare February 20, 2026 20:04
Forgive the user for thinking that --dsn can accept a literal DSN in
addition to an alias.

Internally, recast the confusing variable name "dsn" to "dsn_alias".
@rolandwalker rolandwalker force-pushed the RW/accept-literal-dsns-along-with-aliases branch from 77bc723 to 1d7c207 Compare February 20, 2026 20:05
@rolandwalker rolandwalker merged commit 50135d9 into main Feb 20, 2026
8 checks passed
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