Repository navigation
fix(ruff): recognize current format --check wording - #3964
cestercian wants to merge 2 commits into
Conversation
📊 Automated PR Analysis
SummaryFixes Review Checklist
Linked issues: #3930 Analyzed automatically by wshm · This is an automated analysis, not a human review. |
Protocol-zero-0
left a comment
There was a problem hiding this comment.
Tested the current head against its own parent (8e9aa04) with Ruff 0.17.0 on Linux: 7 scenarios / 21 raw-parent-head commands. The default full-format and concise controls improve as intended. One regression remains for explicitly selected structured format output: failing JSON/JSON-lines checks lose the diagnostic records and fix edits, while the parent preserves them. All tested exit codes remain unchanged; the clean JSON case still returns [].
| // "would reformat" does not match "would be reformatted" — keep both. | ||
| let saw_check_rewrite = output_lower.contains("would reformat:") | ||
| || output_lower.contains("unformatted:") | ||
| || output_lower.contains("would be reformatted"); |
There was a problem hiding this comment.
[P2] Preserve explicitly requested JSON/JSON-lines format diagnostics
The new would be reformatted match also fires inside Ruff JSON records (message: "File would be reformatted"). With bad.py containing x= [1,2,3] and clean.py containing x = [1, 2, 3], run rtk ruff format --check --output-format=json bad.py clean.py: this head replaces the JSON array with Ruff format: files need formatting plus a hint, losing the filename, location and fix edits. Its own parent 8e9aa04 preserves the same parsed records as raw Ruff. The separated --output-format json, 40 dirty files and json-lines cases reproduce this too; exit 1 is preserved.
Please keep the structured format modes on a raw-output path rather than applying the text formatter to their contents, and add a regression check for the failing JSON/JSON-lines cases (a clean [] alone stays valid and would miss this). This follows CONTRIBUTING.md's explicit-format/correctness and transparency rules.
|
Good catch, thanks for testing this so thoroughly. When you pick json / json-lines (or any other non-text |
Protocol-zero-0
left a comment
There was a problem hiding this comment.
Re-tested exact head 693fe83 with Ruff 0.17.0 on Linux: 7 scenarios / 21 raw-previous-head executions. My previous structured-output blocker is fixed: separated/equal JSON options, a clean result, 40 dirty files and JSON-lines preserve the parsed raw diagnostic records, including filenames and fix edits. The prior reviewed head 77fa73f still reproduces the lossy cases. Default full and concise controls keep their exit codes. 17 focused Ruff tests, fmt and clippy with warnings denied pass.
One integration requirement remains before I can approve: the new format_output_is_text helper hand-scans --output-format. The repository's .claude/rules/rust-patterns.md rule 6 requires new argument handling to use the shared arg_tokenizer; please classify this option through that API and retain both spellings and the new failing-output regressions. This is a repository requirement, not a claim that the repaired JSON cases still fail.
GitHub currently reports a conflict with develop. Please keep the rebase and the choice between this PR and #3965 coordinated with the maintainer; they overlap. I tested this exact head in isolation, not a combined merge or other platforms.
| /// (default `full`, `concise`, `grouped`) that `filter_ruff_format` knows how to summarize. | ||
| fn format_output_is_text(args: &[String]) -> bool { | ||
| let mut selected: Option<&str> = None; | ||
| for (i, a) in args.iter().enumerate() { |
There was a problem hiding this comment.
Use the shared arg_tokenizer for this new option classification, as required by Rust Patterns rule 6. This manual loop adds a second parser for split/equal --output-format values. Keep the successful JSON/JSON-lines regression tests while moving the classification to the shared grammar API. The behavioral blocker is fixed in my real-Ruff rerun; this remaining request is about the project’s required argument-handling interface.
ruff 0.12+ `format --check` prints `unformatted:` per file and `N files already formatted` as the summary. The filter still keyed on pre-0.12 `Would reformat:` / `left unchanged`, so the check path fell through to identity (0% compression). Keep both spellings, parse paths after the first colon (Windows-safe), and collect files from the default full diagnostic (` --> path:line:col`) and concise `path:line:col: unformatted:` lines. Fixes rtk-ai#3930 # Conflicts: # src/cmds/python/ruff_cmd.rs
693fe83 to
91b04b5
Compare
|
Thanks for retesting! Rebased onto develop and switched |
Protocol-zero-0
left a comment
There was a problem hiding this comment.
Re-reviewed 91b04b5 with real Ruff 0.17.0 on Linux. This clears my shared-tokenizer blocker: format output selection uses tokenize_grammar, respects split/equal value forms and ignores tokens after --.
Eleven CLI scenarios / 33 raw-previous-head comparisons cover failing/clean/55-file JSON, JSON-lines, SARIF, text controls, a literal filename --output-format=json after the separator, and repeated output-format flags. Structured payloads match raw Ruff, including filenames and fix edits, and the separator filename follows the text-format path. All head exit codes match raw Ruff. Seventeen Ruff unit tests, fmt and clippy (all targets, warnings denied) pass.
One factual clarification to the description: real Ruff 0.17.0 rejects repeated --output-format flags with exit 2 rather than using the last occurrence; this head preserves that rejection and diagnostic. The helper's last-occurrence policy is therefore not a demonstrated CLI behavior for that Ruff version.
This approval covers the refreshed formatter/option parsing. I did not run the full repository suite, rustdoc or other platforms; choosing between this PR and #3965, and integrating the shared-file changes with #3968/#3933, remains the maintainer's decision.
|
Thanks for the approve! Small correction to my earlier note: you're right that real Ruff 0.17 rejects repeated --output-format with exit 2, so "last one wins" was wrong on my part. The tokenizer change just means we pick the value the same way for the single-flag case, and a repeated flag ends up with ruff's own error either way. |
Summary
rtk ruff format --checkcompressed nothing on ruff 0.12+ becausefilter_ruff_formatonly matched pre-0.12 wording (Would reformat:/left unchanged).unformatted:per file andN files already formattedas the summary, plus a full diagnostic. Neither old branch fired, so the filter fell through to identity (0% savings).left unchangedand was already fine; only the--checkpath was dead.Root cause: substring checks were too specific.
"would reformat"does not match"would be reformatted", and"left unchanged"does not match"already formatted".Fix: recognize both generations of wording. Parse
unformatted: <path>by splitting after the first colon (Windows-safe). Also collect paths from the default full diagnostic and from--output-format concise. Keep the pre-0.12 fixtures green.Fixes #3930
Test plan
cargo fmt --all && cargo clippy --all-targets && cargo test --allunformatted:, full diagnostic, concise,already formatted)Notes