Repository navigation
fix(cli): check the database and apply --error-on-warnings with --stdin-file-path - #1082
Merged
Merged
Conversation
…in-file-path check --stdin-file-path skipped check_database_connection, so a configured database that can't be reached silently skipped the checks that need it and the command succeeded. It also failed only on errors and ignored --error-on-warnings. Both now behave like checking a file.
psteinroe
added a commit
that referenced
this pull request
Nov 1, 2026
…#971) Stacked on #945. `check` and `format` each had their own copy of the argument validation and of the path resolution, and `check`, `format` and `dblint` each had their own `enforce_exit_codes`. The copies had drifted apart. **What changed** - `check` and `format` take their files from one `FileTargets` (`paths`, `--stdin-file-path`, `--staged`, `--changed`, `--since`) in `commands/mod.rs`. It validates the combinations, resolves the paths, and yields the nested-configuration paths and the `VcsTargeting`. This replaces `get_files_to_process_with_cli_options`, whose `--since`/`--changed`/`--staged` checks were a third copy and never ran, because both commands validate first. - One `enforce_exit_codes(cli_options, report, category)` for all three commands. The "no files were processed" error only applies to reports that have a traversal, which is what kept `dblint` out of it before. **Behaviour differences between the copies, and which one I kept** - `check --stdin-file-path` with file paths, `--staged` or `--changed` silently ignored them. Worse, with `--staged`/`--changed` it still asked git for the files and then threw the list away. `format` already rejected these combinations, and `check` now does too: ```sh $ echo 'select 1;' | postgres-language-server check --stdin-file-path=virtual.sql migrations/ # before: checks stdin, ignores migrations/ # after: × Incompatible arguments stdin-file-path and PATH ``` `--fix` stays allowed with `--stdin-file-path`. Only `format` rejects `--write` with it, so that check stays in `format`. - `format` ignored `--error-on-warnings`. It now fails on warnings like the other commands. Today `format` only reports errors, so in practice nothing changes yet. **Tests** - `check_stdin_rejects_paths_snapshot` (CLI): same output as the existing `format_stdin_rejects_paths_snapshot`. - Unit tests in `commands/mod.rs`: `--stdin-file-path` rejects paths, `--staged` and `--changed`; with `--error-on-warnings`, warnings fail a `format` report; a report without a traversal (`dblint`) doesn't fail for having no files, while a traversal without files does unless `--no-errors-on-unmatched` is set. - The existing check, format and dblint CLI snapshots are unchanged. **Risks / not covered** - Scripts that pass a path together with `--stdin-file-path` to `check` now fail instead of the path being ignored. - Unchanged, and present in both old copies: when `--changed` or `--staged` finds no files, the paths fall back to the working directory, so the whole project gets checked or formatted. `traverse` explicitly avoids that fallback for VCS targeting, so this looks unintended, but it is a behaviour change of its own and not part of this refactor. **Update:** brought up to date with main and the reworked stack below (merge commit). `check` and `format` now call `CliSession::configure_workspace` (from #991) instead of `prepare_with_config` + `setup_workspace`, and `apply_nested_configurations` no longer takes the CLI configuration, since the session keeps the layers. `CheckArgs` keeps main's new `only`/`skip` next to `targets`, and the `TraversalData` in the tests gets main's new `fixable_diagnostics`/`writes`. No other changes. **Update:** merged main with #1082, which made `check --stdin-file-path` check the database and honour `--error-on-warnings` with its own exit-code logic in `execute/stdin.rs`. The stdin path now goes through `enforce_exit_codes` as well: `run_stdin` returns a `Report` of the diagnostics it printed (no traversal), `check` prepends the database errors to it like for files, and `ExecutionConfig::error_on_warnings` from #1082 is gone. `format --stdin-file-path` uses the same report; its diagnostics are format and syntax errors, so it fails exactly as before. Nested configuration errors are still only printed with stdin. #1082's tests (`check_stdin_fails_when_database_is_unreachable`, `check_stdin_fails_on_warnings_with_error_on_warnings`) pass unchanged.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
check --stdin-file-path(added in #916) handled two things differently from checking files:check_database_connection. With a configured database that can't be reached, the checks that need it were skipped silently and the command exited with 0. Checking the same SQL from a file reportsdatabase/connectionand exits with 1.Errorand ignored--error-on-warnings.printf 'CREATE TABLE t (data json);\n' | postgres-language-server check --disable-db --only=preferJsonb --error-on-warnings --stdin-file-path=virtual.sqlprinted thepreferJsonbwarning and exited with 0.Both were reported in the review of #970, whose branch contained #916 at the time.
What changed
checkchecks the database connection before it branches into stdin or the traversal. With stdin, thedatabase/connectiondiagnostic is printed to stderr next to the configuration errors, and the command fails after printing the checked code.ExecutionConfigcarrieserror_on_warnings, set from the CLI option bycheck. The stdin check fails on warnings when it is set. The traversal still applies the option to its report inenforce_exit_codes, as before.Tests
check_stdin_fails_when_database_is_unreachable: nothing listens on port 1, so stdin fails withdatabase/connection. Before:status: success.check_stdin_fails_on_warnings_with_error_on_warnings: thepreferJsonbwarning succeeds without the flag and fails with it. Before:status: successwith the flag.cargo test -p pgls_cli,cargo clippy --workspace --all-targets -- -D warnings.Not covered
With stdin, the errors of nested configuration files are still only printed, while checking files adds them to the report and its exit code. #971 moves the exit-code handling of the commands into one place, and this PR will conflict with it in
commands/check.rs.