-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Show partial fixability indicator in statistics output #21513
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Show partial fixability indicator in statistics output #21513
Conversation
|
a simple one-liner to see the effect: |
|
Thank you for this contribution Can you say more about why this PR also changes the field name and type? This change is not backward compatible and it's not clear to me how it's related. I see that you wrote this PR with claude. We don't have a policy against using AI but I just want to ask you to review the PR yourself first before we spend time reviewing. I'll put it into draft, you can mark it as ready for review once you think the PR is in a good state. |
The field name and type change happen because the current Current behavior (undocumented):
For example: echo 'from typing import List, AsyncGenerator' | ruff check --select UP035 --statistics --output-format json -Returns Adding This leave us with a dilemma - what to do with
I believe option 3 is best in the long run - users facing an explicit break now (while Ruff is still <1.0) is better than silently getting misleading info. But you know more about Ruff's usage in the wild, so I defer to your judgment here. |
Fair point. Thank you for verifying. That said, I'm not a Rust developer, so I could easily miss Rust-specific idioms, performance considerations, or project conventions. This is my best effort given my experience level. I'll mark it as ready for review now. Happy to iterate on any feedback. |
|
Thanks for the explanation. I ran the command and, as a user, it isn't clear what I also think that fixing the CLI command shouldn't break the CLI output. I suggest keeping |
|
will do.
any input about how to update the summary? some ideas here: |
|
Would it make any sense to report two lines for F811? As in: Just an idea to avoid having to explain another marker in the footer, but maybe it would be confusing if it wasn't 1 of each and the two entries for the same rule sorted farther apart. |
|
another alternative is adding a count (i'm still going with the original idea, but i'm open to inputs from maintainers about their preferred direction) |
When running `ruff --statistics`, diagnostics are now grouped correctly and show three states: - `[*]` when all violations of a rule are fixable - `[~]` when some but not all violations are fixable - `[ ]` when no violations are fixable Previously, the fixability indicator was incorrectly based on the first diagnostic in each group (sorted by fixability), which meant partially fixable rules would always show as unfixable. The JSON output now includes `fixable_count` instead of `fixable` boolean, providing more detailed information about how many violations can be fixed. Closes #20921 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Keep the `fixable` boolean field alongside `fixable_count` in JSON statistics output to avoid breaking existing consumer scripts. The `fixable` field is true when any violation of that code is fixable (i.e., fixable_count > 0). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Test case where fixable_count < count (some but not all violations are fixable) using UP035 with `from typing import List, AsyncGenerator`. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Test case where fixable_count < count (some but not all violations are fixable) using UP035 with `from typing import List, AsyncGenerator`. Shows the [~] marker in text output format. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Add help text explaining that when --statistics is combined with --output-format json, the output includes fixable_count and fixable fields. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Add a legend to the summary message explaining the meaning of the fixability markers: [*] = all fixable, [~] = some fixable. Also adds a text format test for partial fixability using UP035. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
9851623 to
174a341
Compare
|
Thanks for the feedback! I've addressed all the requested changes: Changes Made1. Summary line now explains the symbolsThe summary line for This only appears with Question: Do we want to add the 2. Kept
|
- Add DiagnosticGroup type alias to reduce type complexity - Replace redundant closure with method reference - Simplify nested tuple to flat tuple for clarity 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
|
Also, this should no more be label:breaking |
I sort of like this, except that I would omit the But I'm not a 100% convinced by it. It looks a bit odd. |
|
we can also: or |
|
In my perspective, the count is useful and I also like the conciseness of [~]. |
|
And in case it was missed in the conversation - All other parts of the PR are complete and ready for review, including the current implementation with the [~] format. |
|
Sorry, I had to think it over a bit. I would go with
I also suggest changing |
|
done. example of the current output: |
Replace the boolean `show_legend` parameter with a `SummaryMode` enum to make function calls more self-documenting and explicit. The enum has two variants: - `Default`: Regular summary without legend (shows `[*]` prefix) - `Statistics`: Summary with fixability legend (shows `([*] all, [-] some)`) This improves code readability at call sites, changing unclear boolean literals like `write_summary_text(writer, diagnostics, true)` to explicit `write_summary_text(writer, diagnostics, SummaryMode::Statistics)`. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
|
I'm sorry, but I didn't realize that the summary has variants where it adds another message in parentheses and the double parenthesizing looks sort of bad. What do you think of: and assume that users will be able to infer that or |
|
If you are asking for my input, I feel that a need to infer the meaning is less ideal. What do you think about: It also shortens the line length, which is a plus. (I don't think it's perfect, but it's the best I've found yet) |
|
I like that |
|
Thanks for the feedback! I looked at the code more carefully and realized that I need another input before continuing. The issue is that this change would create an inconsistency with other output modes. i.e., compare with those examples Examples:
(note: when we don't use
OptionsDo you think we should:
I'm leaning toward option 1, but I wanted to get your input before. WDYT? |
|
Also, in one case the output is |
|
I'm sort of leaning towards leaving it as is (the legend) and assuming that users can infer the meaning of
I'm fine landing it in this PR |
|
Done :) |
it's contained in the last commit, so we can inspect it separately or move it to a different PR if needed |
MichaReiser
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you
|
|
Thank you too! |
Summary
Fixes #20921
When running
ruff --statistics, the fixability indicator now correctly shows whether violations are fully, partially, or not fixable:[*]when all violations of a rule are fixable[~]when some but not all violations are fixable[ ]when no violations are fixableThe bug occurred because diagnostics were sorted by fixability before grouping, causing the first (unfixable) diagnostic to always be used as the representative for the group.
JSON API Change
The JSON statistics output now includes
fixable_countinstead offixableboolean:{ "code": "UP035", "name": "deprecated-import", "count": 2, "fixable_count": 1 }Question for maintainers: Should we keep the old
fixableboolean field for backwards compatibility? If so, should it indicate "any fixable" (fixable_count > 0) or "all fixable" (fixable_count == count)?Test plan
show_statistics_jsonandshow_statistics_json_unsafe_fixesQuestion: Are additional tests needed, e.g., for the partial fixability indicator
[~]?🤖 Generated with Claude Code