Repository navigation
fix(ruff): summarize format --check output for ruff >= 0.16 wording (#3930) - #3965
anujsingh-cse wants to merge 2 commits into
Conversation
📊 Automated PR Analysis
SummaryFixes filter_ruff_format to recognize ruff >= 0.16's new format --check output wording (unformatted: / already formatted / --> path:row:col pointer lines) while retaining support for the old pre-0.12 wording, restoring the intended output-summarization savings. Adds two unit tests covering the new wording. Review Checklist
Linked issues: #3930 Analyzed automatically by wshm · This is an automated analysis, not a human review. |
cc5d361 to
6a000cf
Compare
|
Rebased onto latest \develop\ so it's up to date and mergeable. Happy to adjust anything reviewers would like. |
6a000cf to
02c286c
Compare
|
Rebased onto current develop and fixed the CI failure. The branch was 300+ commits behind a develop that has since repaired broken rustdoc intra-doc links (the rustdoc gate failed on the stale base), and current stable clippy now flags the nested All local gates pass against current develop: |
Protocol-zero-0
left a comment
There was a problem hiding this comment.
Tested this rebased head with real Ruff 0.17.0 on Linux. The default full-format mixed-file check now reports the dirty file correctly, and the 12 Ruff unit tests pass.
There is one blocking correctness issue in the supported --output-format concise path: a mixed clean/dirty set is summarized as all files formatted correctly. This is newly introduced relative to current develop (a4cddfc), which preserves the actual concise diagnostics. The process still exits 1, but every unformatted-file diagnostic disappears.
I compared seven real CLI cases (28 raw/develop/#3964/#3965 commands): full mixed, concise mixed, concise 40-dirty/1-clean, concise dirty-only, concise clean-only, JSON output and invalid syntax. Child exit statuses remain 0/1/2 as appropriate. This is targeted review evidence; I did not run the full quality pipeline or other operating systems.
| // Check if all files are formatted | ||
| if files_to_format.is_empty() && output_lower.contains("left unchanged") { | ||
| if files_to_format.is_empty() | ||
| && (output_lower.contains("left unchanged") || output_lower.contains("already formatted")) |
There was a problem hiding this comment.
[P1] Preserve concise-format failures before returning the all-formatted summary
Ruff supports ruff format --check --output-format concise bad.py clean.py. With bad.py containing x= [1,2,3] and clean.py containing x = [1, 2, 3], Ruff 0.17.0 emits:
bad.py:1:2: unformatted: File would be reformatted
1 file would be reformatted, 1 file already formatted
The concise diagnostic has no following --> line, so the new parser leaves files_to_format empty. The new already formatted condition then returns Ruff format: All files formatted correctly, discarding the dirty file. The same happens with 40 dirty files and one clean file. Both raw Ruff and RTK still exit 1; the bug is the misleading filtered body, not the exit status.
Please parse the concise location form or conservatively preserve the raw failure output when a reformat marker/count is present. An empty collected-path list must not establish that all files are clean. A mixed concise-output regression test would cover this branch.
Real `ruff format --check --output-format concise` (0.17.0) on a mixed
clean/dirty set ends with a summary that also names already-formatted
files:
dirty.py:1:6: unformatted: File would be reformatted
1 file would be reformatted, 1 file already formatted
The concise per-file lines never reach the `-->` pairing, so
`files_to_format` stays empty, and the all-formatted guard fired on the
"already formatted" half of the summary alone — reporting every
unformatted-file diagnostic as gone while the process still exited 1.
The clean conclusion is now only drawn when the output contains no
reformatted indication at all: no "unformatted" and no "would be
reformat". Mixed concise runs fall through and pass the raw
diagnostics through, matching develop.
|
Fixed in c9dc768. Root cause confirmed on real Ruff 0.17.0: the mixed concise run ends with The clean conclusion now also requires that the output contains no reformatted indication at all — no Re-verified against real Ruff 0.17.0 through the built rtk binary, matching your seven-case shape:
All 14 ruff_cmd unit tests pass; |
Protocol-zero-0
left a comment
There was a problem hiding this comment.
Independently rechecked c9dc768 with real Ruff 0.17.0 against its exact parent (02c286c).
Seven cases / 21 raw-parent-head executions cover full-format mixed, concise mixed/many/dirty/clean, explicit JSON, and a syntax error. The prior false-clean conclusion reproduces for concise mixed/many; this head preserves those diagnostics verbatim instead. Dirty-only stays correct, full-format mixed remains summarized, the clean control exits 0, JSON remains parseable with the same payload, and the syntax-error diagnostic survives. Every head exit code matches raw Ruff (including 1 for formatting failures and 2 for invalid syntax).
All 14 focused Ruff tests pass, and cargo fmt --all --check passes. This clears my earlier concise-output blocker. Approval covers this narrow wording/guard fix and the local Linux checks above; it does not select between this PR and the overlapping #3964, or replace the maintainer's integration decision. I did not rerun the full repository suite or other platforms.
Closes #3930
Problem
filter_ruff_formatkeys only off pre-0.12 ruff strings:"would reformat:"and"left unchanged". On ruff 0.16.6format --checkprintsunformatted: File would be reformattedper file (with the path on a following--> path:row:colpointer line) andN files already formattedas the summary. Neither branch matched, so the filter fell through to an identity passthrough — 0% savings on a path meant to hit the 20% floor.Repro (ruff 0.16.6):
Fix
Extend
filter_ruff_formatto recognise the current wording while keeping the old spellings:unformatted:and extract the path from the following--> path:row:colline, alongside the existingWould reformat: pathalready formattedalongside the existingleft unchanged!files_to_format.is_empty()(both old and new per-file issuers populate it) plus the existingwould reformatfallbackTests
Added two unit tests using the new wording (captured from a current ruff):
test_filter_ruff_format_all_formatted_ruff_016_wording—"5 files already formatted"test_filter_ruff_format_needs_formatting_ruff_016_wording— theunformatted:+--> path:row:coltwo-line formExisting old-wording tests unchanged and still pass;
cargo fmt --checkandcargo clippy --all-targetsclean; 19 ruff tests pass. Verified end-to-end that ruff 0.16.6format --checkoutput (156 raw bytes) now flattens to a compact summary instead of passing through.