-
Notifications
You must be signed in to change notification settings - Fork 1.6k
feat: 'ruff rule' provides more easily parsable JSON ouput #20168
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
feat: 'ruff rule' provides more easily parsable JSON ouput #20168
Conversation
ntBre
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.
Thanks! This looks good to me. I'm surprised we don't have any tests for this either, but we should add some. I found a couple of ruff rule tests here:
ruff/crates/ruff/tests/integration_test.rs
Lines 949 to 952 in c3f1f0b
| #[test] | |
| fn rule_f401() { | |
| assert_cmd_snapshot!(ruff_cmd().args(["rule", "F401"])); | |
| } |
but none for the JSON format. This could be a good place to add one.
I also think this is a breaking change that we might need to gate behind preview. We could theoretically try to squeeze it into the minor release tomorrow, but I'd be interested in getting @MichaReiser's thoughts next week when he's back from PTO anyway, instead of trying to rush it.
crates/ruff_linter/src/codes.rs
Outdated
| use ruff_db::diagnostic::SecondaryCode; | ||
| use strum_macros::EnumIter; |
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.
tiny nit: I'd group these up above with the serde import. Unfortunately stable rustfmt still doesn't handle these groups automatically.
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.
done
|
|
You're welcome. I'll add the tests, but I have no time until next week. |
|
Since For stabilizing features, we can just open an issue and add it to the next Milestone on GitHub. We go through all of those before every minor release and handle the stabilizations. In the linter we collect helper functions for preview behaviors in |
crates/ruff/src/commands/rule.rs
Outdated
| explanation: Option<&'a str>, | ||
| preview: bool, | ||
| fix: FixAvailability, | ||
| group: RuleGroup, |
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.
I suggest renaming this to status. It might otherwise be mistaken for a rule category (something we want to introduce in the future).
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.
done
crates/ruff/src/commands/rule.rs
Outdated
| fix, | ||
| explanation: rule.explanation(), | ||
| preview: rule.is_preview(), | ||
| fix: rule.fixable(), |
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.
I agree that this is an improvement, but I'm not sure if it's worth going through the complications of adding a preview flag (which might even be more annoying for upstream tools than an outright breaking change because they now need to handle both formats if the user set preview = true in their configuration).
I'm inclined to just leave it as is OR introduce a new field fix_availability and deprecate the old field.
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.
I'd prefer to go with the second option. So the old field should be deprecated in the next minor release, right?
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.
Yes, we would deprecate or outright remove it in the next minor. But again, I'm not sure what the value of doing so is. It only breaks downstream users. So I expect the field to stick around for a long time :)
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.
I thought having the enum values instead of entire sentences like "Fix is sometimes available." would allow users to parse the json more easily, which was the point raised by the issue
We could also leave both fields, even if this would mean some duplication...
Not sure what's best... Let me know what you prefer :)
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.
I'm fine with reverting the change or adding a second field
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.
We probably need to restore the preview field in that case too right? I like the idea of leaving the existing fields untouched (restoring preview and the string fix field) and adding the status and fix_availability fields, which should be easier to match exhaustively.
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.
Yes, we need to restore any breaking change.
|
Hello everyone, But anyway, I've just updated the branch. So I left all the old fields untouched, I just added new ones. I added basic integration tests for the "ruff rule" with "--output-json" command, very simillar to the existing ones testing "ruff rule". Since there's no breaking change anymore, it's fine to not put the new fields behind a "--preview" CLI flag, right? |
|
No worries. I sort of forgot about it too. I'll take a look now |
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
Summary
Not sure if it entirely closes #9891, but it's at least a step in the right direction
This change improves the JSON output of the "ruff rule" command.
FixAvailabilityenumRuleGroupenum (stable, preview, deprecated, removed). It's more accurate and parsable that wayTest Plan
I saw no tests for the "ruff rule" command (maybe I missed them?)
So I tested manually: