Conversation
Diagnostic diff on typing conformance testsNo changes detected when running ty on typing conformance tests ✅ |
|
|
|
I'm planning to pick this up again. This should now be easier with #22083 |
d464344 to
d95a3e9
Compare
d95a3e9 to
f887e81
Compare
fbe2478 to
9b37298
Compare
This comment was marked as outdated.
This comment was marked as outdated.
cb5c2ce to
86db9a7
Compare
--add-ignore CLI option
76d6adc to
3f9bf85
Compare
| /// Apply a series of fixes to `File` and returns the updated source code along with the source map. | ||
| /// | ||
| /// Returns an error if not all fixes were applied because some fixes are overlapping. | ||
| fn apply_fixes(source: &str, mut fixes: Vec<Fix>) -> Result<FixedCode, FixedCode> { |
There was a problem hiding this comment.
This is a copy of the same function in Ruff
| } else { | ||
| // If there are any other file level diagnostics, call `check_file` to re-compute them | ||
| // with updated ranges. | ||
| let diagnostics = project.check_file(db, file); |
There was a problem hiding this comment.
We'll always need to call check_file when implementing --fixes. It will be interesting how slow that will be
There was a problem hiding this comment.
Isn't this somewhat uncommon, though? My understanding (likely wrong) is that these would only be... unused ignore comments, and syntax errors (or other non-suppressible diagnostics)?
There was a problem hiding this comment.
Today yes. These are diagnostics like reveal_type, IO errors (unlikely we ever make it here). However, this will be much more common once we have a --fix mode because most diagnostics don't have a fix. At the other hand, it might not be an issue because there are much fewer files that we change. So it might be fine after all.
| } else { | ||
| // If there are any other file level diagnostics, call `check_file` to re-compute them | ||
| // with updated ranges. | ||
| let diagnostics = project.check_file(db, file); |
There was a problem hiding this comment.
Isn't this somewhat uncommon, though? My understanding (likely wrong) is that these would only be... unused ignore comments, and syntax errors (or other non-suppressible diagnostics)?
|
|
||
| /// Adds sub diagnostics that tell the user that this is a bug in ty | ||
| /// and asks them to open an issue on GitHub. | ||
| pub fn add_bug_sub_diagnostics(&mut self, url_encoded_title: &str) { |
There was a problem hiding this comment.
As an API, I feel like it would make more sense if this took the un-encoded title, and the function URL-encoded it for you. But I assume that means we'd need to depend on a URL-encoding implementation that we can otherwise omit.
There was a problem hiding this comment.
I agree in terms of API design, but it felt overkill to introduce a new URL encoding dependency for the few places where we call this function
87551b9 to
0745b08
Compare
|
| Lint rule | Added | Removed | Changed |
|---|---|---|---|
invalid-return-type |
1 | 0 | 9 |
invalid-assignment |
0 | 0 | 5 |
invalid-argument-type |
0 | 0 | 4 |
| Total | 1 | 0 | 18 |
Summary
This PR adds a new
--add-ignoreCLI option that makes ty addty: ignore[codes]comments to suppress any lint diagnostics (or add the codes to existing comments).There's quite a lot going on in this PR:
source_textso that it uses the fixed source instead of the source on disk. This is required to check whether there are any new parse diagnostics after fixing the file (and computing the remaining non-rule diagnostics). This is a case where Salsa's speculative execution would be very handy :)WritableSystemto write arbitrary byte arrays (necessary for notebooks)This PR also lays the foundation for a
--fixoption. In fact, most of it has been written so that applying arbitrary fixes should be relatively straightforward. The main wringkle of--fixis that ty needs to callcheck_filemultiple times and apply all fixes until there are no new fixes left.Closes astral-sh/ty#913
Test Plan
I tested this new feature on a few larger projects:
I verified that there are:
--add-ignorecheckafter returns no new diagnostics.