-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Use structs for JSON serialization #19270
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
Conversation
|
| url: Option<String>, | ||
| } | ||
|
|
||
| struct ExpandedEdits<'a> { |
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 leaning towards reverting the last commit. The old implementation carefully avoided allocating a new vector for the edits. I also don't think that changing this is necessary for making this more typed. We can just replace the
let value = json!({
"content": edit.content().unwrap_or_default(),
"location": location_to_json(location),
"end_location": location_to_json(end_location)
});and initialize and then serialize said struct instead.
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.
Makes sense, reverted!
This reverts commit 4733e0a.
## Summary Another output format like #19133. This is the [reviewdog](https://github.com/reviewdog/reviewdog) output format, which is somewhat similar to regular JSON. Like #19270, in the first commit I converted from using `json!` to `Serialize` structs, then in the second commit I moved the module to `ruff_db`. The reviewdog [schema](https://github.com/reviewdog/reviewdog/blob/320a8e73a94a09248044314d8ca326a6cd710692/proto/rdf/jsonschema/DiagnosticResult.json) seems a bit more flexible than our JSON schema, so I'm not sure if we need any preview checks here. I'll flag the places I wasn't sure about as review comments. ## Test Plan New tests in `rdjson.rs`, ported from the old `rjdson.rs` module, as well as the new CLI output tests. --------- Co-authored-by: Micha Reiser <micha@reiser.io>
Summary
See #19133 (comment) for recent discussion. This PR moves to using structs for the types in our JSON output format instead of the
json!macro.I didn't rename any of the
messagereferences because that should be handled when rebasing #19133 onto this.My plan for handling the
previewbehavior with the new diagnostics is to use a wrapper enum. Something like:Initially I thought I could use a
&dyn Serializefor the affected fields, but I see thatSerializeisn't dyn-compatible in testing this now.Test Plan
Existing tests. One quirk of the new types is that their fields are in alphabetical order. I guess
json!sorts the fields alphabetically? The tests were failing before I sorted the struct fields.Other formats
It looks like the
rdjson,sarif, andgitlabformats also usejson!, so if we decide to merge this, I can do something similar for those before moving them to the new diagnostic format.