-
Notifications
You must be signed in to change notification settings - Fork 1.7k
[ty] Add new diagnosticMode: off
#22073
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
Diagnostic diff on typing conformance testsNo changes detected when running ty on typing conformance tests ✅ |
|
dhruvmanila
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.
This looks good but I'd prefer the approach to change the capabilities instead (the one that you've mentioned in the description). I'm also fine if you want to do it as a follow-up.
I was wondering if we need to change anything for db.set_check_mode here:
ruff/crates/ty_server/src/session.rs
Lines 550 to 558 in fee4e2d
| if let Some(global_options) = combined_global_options { | |
| let global_settings = global_options.into_settings(); | |
| if global_settings.diagnostic_mode().is_workspace() { | |
| for project in self.projects.values_mut() { | |
| project.db.set_check_mode(CheckMode::AllFiles); | |
| } | |
| } | |
| self.global_settings = Arc::new(global_settings); | |
| } |
but I think it's fine now given that changing the configuration value requires a server restart.
| _client: &Client, | ||
| params: DocumentDiagnosticParams, | ||
| ) -> Result<DocumentDiagnosticReportResult> { | ||
| if snapshot.global_settings().diagnostic_mode().is_off() { |
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.
Is this still required?
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, for clients that don't support dynamic registration
283824c to
cde5218
Compare
Summary
This PR adds a new
diagnosticMode: off, for users that want to use ty's LSP functionality only (go to def, ...) but don't want to see any type errors.I took the easy way here where the LSP returns an empty response for pull diagnostics rather than changing the registered capabilities. Happy to pursue that if people think it's worth the time.
We may want to add a separate
disableSyntaxErrorsoption for users that want to use ty alongside other Python LSPs. I created a new issue for that.Closes astral-sh/ty#1755
Test Plan
Added tests
Screen.Recording.2025-12-19.at.11.04.32.mov