-
Notifications
You must be signed in to change notification settings - Fork 1.9k
[ty] Fix handling of non-Python text documents #23704
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
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 |
|---|---|---|
|
|
@@ -429,6 +429,8 @@ def foo() -> str: | |
| insta::assert_debug_snapshot!(diagnostics); | ||
|
|
||
| server.close_text_document(foo); | ||
| let diagnostics = server.await_notification::<PublishDiagnostics>(); | ||
| insta::assert_debug_snapshot!(diagnostics); | ||
|
Member
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. I believe this is is where we should have been awaiting the diagnostic notification before. But since we didn't send one back for the |
||
|
|
||
| let params = DidOpenTextDocumentParams { | ||
| text_document: TextDocumentItem { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,21 @@ | ||
| --- | ||
| source: crates/ty_server/tests/e2e/publish_diagnostics.rs | ||
| expression: diagnostics | ||
| --- | ||
| PublishDiagnosticsParams { | ||
| uri: Url { | ||
| scheme: "file", | ||
| cannot_be_a_base: false, | ||
| username: "", | ||
| password: None, | ||
| host: None, | ||
| port: None, | ||
| path: "<temp_dir>/src/foo", | ||
| query: None, | ||
| fragment: None, | ||
| }, | ||
| diagnostics: [], | ||
| version: Some( | ||
| 1, | ||
| ), | ||
| } |
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: We could still skip publishing diagnostics for non-python files (I think?). But IMO, it's not a big deal if we still call it because
project.check_filewill bail early for non Python files.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.
Yeah that was exactly my thought. It seemed fragile to repeat the language ID check here, since there are other reasons why a file might not get added to a project, e.g., when
project.is_file_included(db, system_path)returnsfalse. So if we wanted to do this, I think we'd want to bubble up a flag about whether the file was actually added to the project or not. And at that point it didn't seem worth doing.