-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Move GitLab output rendering to ruff_db
#20117
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
it was added specifically for gitlab in #3475 and isn't used anywhere else, so this prepares to move it with the gitlab module to ruff_db
this is a clean move of all the Serialize code and the two helper methods
instead of having everything in the if-let, map the part that actually needs the span, falling back on a default value as required by the spec
this won't affect Ruff, where all diagnostics are still errors, but this should work in ty
Diagnostic diff on typing conformance testsNo changes detected when running ty on typing conformance tests ✅ |
|
|
fn relativize_path_to<P: AsRef<Path>, R: AsRef<Path>>(path: P, project_root: R) -> String { | ||
format!( | ||
"{}", | ||
pathdiff::diff_paths(&path, project_root) |
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 suspect that we could just use our render::relativize_path
helper here and drop this dependency, but I kept it around for now. We don't have any existing tests covering the CI_PROJECT_DIR
case.
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.
Dropping a dependency sounds great. :-)
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 think I'll open a follow-up issue here. I'm pretty sure that we can drop this entirely and just use the CWD-relative path, but I'm not 100% sure and don't want to break GitLab users.
It's a one-function dependency, so we could also vendor it. The behavior seems a bit different from our relativize_path
if the project_root
isn't a prefix, if we really need that.
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.
LGTM.
Possible idea for next time: if you have 3 logical groupings of commits, it might make sense to just squash that into 3 commits. Then you can write a commit message for that grouping and it should hopefully make review easier. (And avoid needing to write out the links for the individual groups of commits. Which I assume will go bad if you rebase.)
diagnostics, | ||
resolver: self.resolver, | ||
project_dir: std::env::var("CI_PROJECT_DIR").ok().as_deref(), | ||
}) |
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.
How come serde_json::json!
here instead of serde_json::to_string
? I'd expect the latter to be faster since it doesn't have to construct an intermediate serde_json::Value
, but that's just a wild guess.
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 guess it's a shame we can't just stream this right to a std::fmt::Write
without going through some intermediate value first.
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.
json!
hides the unwrap
call in the macro and we used it in the other JSON formats, but those aren't very good reasons 😬
But yeah, the Emitter
s in ruff_linter
take std::io::Write
s instead of std::fmt::Write
s, making this a little easier. We added an adapter in the junit
PR, but I haven't used it any of the other formats yet:
ruff/crates/ruff_db/src/diagnostic/render/junit.rs
Lines 150 to 154 in ef0586d
struct FmtAdapter<'a> { | |
fmt: &'a mut dyn std::fmt::Write, | |
} | |
impl std::io::Write for FmtAdapter<'_> { |
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 switched it over to to_string
for now!
fn relativize_path_to<P: AsRef<Path>, R: AsRef<Path>>(path: P, project_root: R) -> String { | ||
format!( | ||
"{}", | ||
pathdiff::diff_paths(&path, project_root) |
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.
Dropping a dependency sounds great. :-)
Thanks for the review!
Will do, sorry about that. I thought each commit would be helpful initially, but then they got a bit too small. I should have squashed after identifying the groups. It's definitely a pain to maintain the links! |
this also reorders the fields in the snapshots, but that should be fine
* main: Fix mdtest ignore python code blocks (#20139) [ty] add support for cyclic legacy generic protocols (#20125) [ty] add cycle detection for find_legacy_typevars (#20124) Use new diff rendering format in tests (#20101) [ty] Fix 'too many cycle iterations' for unions of literals (#20137) [ty] No boundness analysis for implicit instance attributes (#20128) Bump 0.12.11 (#20136) [ty] Benchmarks for problematic implicit instance attributes cases (#20133) [`pyflakes`] Fix `allowed-unused-imports` matching for top-level modules (`F401`) (#20115) Move GitLab output rendering to `ruff_db` (#20117) [ty] Evaluate reachability of non-definitely-bound to Ambiguous (#19579) [ty] Introduce a representation for the top/bottom materialization of an invariant generic (#20076) [`flake8-async`] Implement `blocking-http-call-httpx` (`ASYNC212`) (#20091) [ty] print diagnostics with fully qualified name to disambiguate some cases (#19850) [`ruff`] Preserve relative whitespace in multi-line expressions (`RUF033`) (#19647)
## Summary This wires up the GitLab output format moved into `ruff_db` in #20117 to the ty CLI. While I was here, I made one unrelated change to the CLI docs. Clap was rendering the escapes around the `\[default\]` brackets for the `full` output, so I just switched those to parentheses: ``` --output-format <OUTPUT_FORMAT> The format to use for printing diagnostic messages Possible values: - full: Print diagnostics verbosely, with context and helpful hints \[default\] - concise: Print diagnostics concisely, one per line - gitlab: Print diagnostics in the JSON format expected by GitLab Code Quality reports ``` ## Test Plan New CLI test, and a manual test with `--config 'terminal.output-format = "gitlab"'` to make sure this works as a configuration option too. I also tried piping the output through jq to make sure it's at least valid JSON
## Summary This PR is a first step toward adding a GitLab output format to ty. It converts the `GitlabEmitter` from `ruff_linter` to a `GitlabRenderer` in `ruff_db` and updates its implementation to handle non-Ruff files and diagnostics without primary spans. I tried to break up the changes here so that they're easy to review commit-by-commit, or at least in groups of commits: - [preparatory changes in-place in `ruff_linter` and a `ruff_db` skeleton](https://github.com/astral-sh/ruff/pull/20117/files/0761b73a615537fe75fc54ba8f7d5f52c28b2a2a) - [moving the code over with no implementation changes mixed in](https://github.com/astral-sh/ruff/pull/20117/files/0761b73a615537fe75fc54ba8f7d5f52c28b2a2a..8f909ea0bb0892107824b311f3982eb3cb1833ed) - [tidying up the code now in `ruff_db`](https://github.com/astral-sh/ruff/pull/20117/files/9f047c4f9f1c826b963f0fa82ca2411d01d6ec83..e5e217fcd611ab24f516b2af4fa21e25eadc5645) This wasn't strictly necessary, but I also added some `Serialize` structs instead of calling `json!` to make it a little clearer that we weren't modifying the schema (e4c4bee). I plan to follow this up with a separate PR exposing this output format in the ty CLI, which should be quite straightforward. ## Test Plan Existing tests, especially the two that show up in the diff as renamed nearly without changes
## Summary This wires up the GitLab output format moved into `ruff_db` in astral-sh#20117 to the ty CLI. While I was here, I made one unrelated change to the CLI docs. Clap was rendering the escapes around the `\[default\]` brackets for the `full` output, so I just switched those to parentheses: ``` --output-format <OUTPUT_FORMAT> The format to use for printing diagnostic messages Possible values: - full: Print diagnostics verbosely, with context and helpful hints \[default\] - concise: Print diagnostics concisely, one per line - gitlab: Print diagnostics in the JSON format expected by GitLab Code Quality reports ``` ## Test Plan New CLI test, and a manual test with `--config 'terminal.output-format = "gitlab"'` to make sure this works as a configuration option too. I also tried piping the output through jq to make sure it's at least valid JSON
## Summary This is the GitHub analog to #20117. This PR prepares to add a GitHub output format to ty by moving the implementation from `ruff_linter` to `ruff_db`. Hopefully this one is a bit easier to review commit-by-commit. Almost all of the refactoring this time is in the first commit, then the second commit adds the new `OutputFormat` variant and moves the file into `ruff_db`. The third commit is just a small touch up to use a private method that accommodates ty files so that we can run the tests and update/move the snapshots. I had to push a fourth commit to fix and test diagnostics without a span/file. ## Test Plan Existing tests
Summary
This PR is a first step toward adding a GitLab output format to ty. It converts the
GitlabEmitter
fromruff_linter
to aGitlabRenderer
inruff_db
and updates its implementation to handle non-Ruff files and diagnostics without primary spans. I tried to break up the changes here so that they're easy to review commit-by-commit, or at least in groups of commits:ruff_linter
and aruff_db
skeletonruff_db
This wasn't strictly necessary, but I also added some
Serialize
structs instead of callingjson!
to make it a little clearer that we weren't modifying the schema (e4c4bee).I plan to follow this up with a separate PR exposing this output format in the ty CLI, which should be quite straightforward.
Test Plan
Existing tests, especially the two that show up in the diff as renamed nearly without changes