-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Hide empty snippets for full-file diagnostics #19653
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
Changes from all commits
7b1dbc1
864c056
c35d34f
c64a7e5
739b35b
1e6c99f
b59ddc7
f3e0e2a
2cfad84
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -712,6 +712,11 @@ pub struct Annotation { | |||||||||||
| is_primary: bool, | ||||||||||||
| /// The diagnostic tags associated with this annotation. | ||||||||||||
| tags: Vec<DiagnosticTag>, | ||||||||||||
| /// Whether this annotation is a file-level or full-file annotation. | ||||||||||||
| /// | ||||||||||||
| /// When set, rendering will only include the file's name and (optional) range. Everything else | ||||||||||||
| /// is omitted, including any file snippet or message. | ||||||||||||
| is_file_level: bool, | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| impl Annotation { | ||||||||||||
|
|
@@ -730,6 +735,7 @@ impl Annotation { | |||||||||||
| message: None, | ||||||||||||
| is_primary: true, | ||||||||||||
| tags: Vec::new(), | ||||||||||||
| is_file_level: false, | ||||||||||||
| } | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
|
|
@@ -746,6 +752,7 @@ impl Annotation { | |||||||||||
| message: None, | ||||||||||||
| is_primary: false, | ||||||||||||
| tags: Vec::new(), | ||||||||||||
| is_file_level: false, | ||||||||||||
| } | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
|
|
@@ -811,6 +818,21 @@ impl Annotation { | |||||||||||
| pub fn push_tag(&mut self, tag: DiagnosticTag) { | ||||||||||||
| self.tags.push(tag); | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| /// Set whether or not this annotation is file-level. | ||||||||||||
| /// | ||||||||||||
| /// File-level annotations are only rendered with their file name and range, if available. This | ||||||||||||
| /// is intended for backwards compatibility with Ruff diagnostics, which historically used | ||||||||||||
| /// `TextRange::default` to indicate a file-level diagnostic. In the new diagnostic model, a | ||||||||||||
| /// [`Span`] with a range of `None` should be used instead, as mentioned in the `Span` | ||||||||||||
| /// documentation. | ||||||||||||
| /// | ||||||||||||
| /// TODO(brent) update this usage in Ruff and remove `is_file_level` entirely. See | ||||||||||||
| /// <https://github.com/astral-sh/ruff/issues/19688>, especially my first comment, for more | ||||||||||||
| /// details. | ||||||||||||
| pub fn set_file_level(&mut self, yes: bool) { | ||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we document the meaning of a file level annotation here too
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good idea, I added a note here about ruff/crates/ruff_db/src/diagnostic/mod.rs Lines 1080 to 1084 in e7e7b7b
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should this get a TODO that it ought to be removed in favor of Ruff diagnostics being updated to use a
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah that's a good point. I added an explicit TODO in the code here and also wrote some notes from looking into the options today: #19688 (comment). In short, I thought we would handle this with a It might be a bit tangential to the main issue in #19688, but at least it's recorded somewhere, and I can spin it off later if needed.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah this at least gives us a possible path toward eventually merging back with upstream. |
||||||||||||
| self.is_file_level = yes; | ||||||||||||
| } | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| /// Tags that can be associated with an annotation. | ||||||||||||
|
|
||||||||||||
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.
Nit: Would it make sense to add an assertion that the snippet has no body (e.g no associated message?)
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.
Sure, some kind of assertion makes sense to me. I think we can assert that the snippet source is empty, I'll just have to rework the test to avoid using the builder, which expects a non-empty snippet. It looks like Ruff's real diagnostics that will go through this path should already have empty snippets. We could also check that the annotation labels are empty (the text after
^^^), but I think those are always empty for us right now.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
RenderableSnippet::newmight try to expand the snippet to the usual number of context lines, unless we also checkis_file_levelthere. We might have to remove or update the assertion when running this in #19415, but I'll add it for now. It'll be a good reminder to revisit it.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.
Oh, maybe that's in line with your suggestion about applying this per-rule. I guess we shouldn't render one of these except in cases where the snippet isn't available, like an
io-error!