Skip to content

Respect --no-show-warnings#1737

Merged
rolandwalker merged 1 commit intomainfrom
RW/respect-no-show-warnings
Mar 24, 2026
Merged

Respect --no-show-warnings#1737
rolandwalker merged 1 commit intomainfrom
RW/respect-no-show-warnings

Conversation

@rolandwalker
Copy link
Copy Markdown
Contributor

Description

--no-show-warnings should override the show_warnings setting in
~/.myclirc.

I don't think this was introduced by #1732, but that PR was bug-for-bug compatible.

To solve the bug, we need for show_warnings to allow a None value, rather than only a bool. None represents no relevant CLI argument being given.

Stuffing everything into connect() isn't great, but this is how other CLI arguments are reconciled with configuration, so it makes sense pending more refactoring of main.py.

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 .

--no-show-warnings should override the show_warnings setting in
~/.myclirc.

To solve the bug, we need for show_warnings to allow a None value,
rather than only a bool.  None represents no relevant CLI argument being
given.
@rolandwalker rolandwalker self-assigned this Mar 23, 2026
@github-actions
Copy link
Copy Markdown

  1. Medium: Backward-compatibility regression in MyCli constructor

    • The PR removes show_warnings from MyCli.__init__ (main.py).
    • Any external consumer doing MyCli(show_warnings=True/False) will now crash with TypeError: unexpected keyword argument.
    • Suggested fix: keep show_warnings: bool | None = None in __init__, and apply it only when not None (same precedence pattern used in connect at main.py).
  2. Missing test coverage for API compatibility

    • New tests validate CLI precedence (--no-show-warnings) (test_main.py), but there is no test guarding constructor compatibility.
    • Suggested test: instantiate MyCli(show_warnings=False/True, myclirc=...) and assert it initializes without error (or explicitly document/remove this API if intentional).

No security issues found in the introduced diff.

Assumption: this review is static; I did not run the db integration tests locally.

@rolandwalker rolandwalker merged commit 289fbdd into main Mar 24, 2026
10 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