[ty] Publish diagnostics for all open files (#1652)#22335
[ty] Publish diagnostics for all open files (#1652)#22335ficd0 wants to merge 1 commit intoastral-sh:mainfrom
Conversation
ficd0
requested review from
MichaReiser,
carljm,
dcreager and
sharkdp
as code owners
|
Sorry, not sure why the issue wasn't automatically linked. The issue in question: astral-sh/ty#1652 |
This commit fixes issue astral-sh#1652, improving support for LSP clients that don't support pull diagnostics by doing the following: - Implemented support for the `didSave` notification type: - Registered handler and advertise this capability on server init - Updated tests: init snapshots now include the new capability - On `didSave` for any document, we call `publish_diagnostics_if_needed` on any open documents.
AlexWaygood
added
ty
MichaReiser
left a comment
MichaReiser
left a comment
There was a problem hiding this comment.
Thanks for working on this.
We'll need to extend this a little to properly support notebooks. We should also add an E2E test (ideally including a notebook and a regular text document), see
| for document in session.text_document_handles() { | ||
| publish_diagnostics_if_needed(&document, session, client); | ||
| } |
There was a problem hiding this comment.
I think simply iterating over text_document_handles here will result in duplicate diagnostics for notebooks because each cell is represented as its own text document.
I think we want to add a new method to session that returns an iterator over all file_documents. That is, all TextDocument where notebook is None and all NotebookDocuments.
|
Thank you for the review and the helpful comments. I will get to work on implementing these changes when I'm able. If I'm confused about anything, I'll ping you here if you don't mind. Cheers! |
|
@MichaReiser is there anything else I should keep in mind re notebooks? I've actually never used them so I kind of forgot the feature even existed, hence their omission from the first draft. Mainly, if I add an appropriate end to end test that covers both regular files and notebooks, do I still need to manually test with notebooks in my editor, too, or is the test enough for us to be confident it works? |
An E2E test should be sufficient. I'm also more than happy to give it a try if I think any extra testing is necessary |
3 participants
This commit fixes astral-sh/ty#1652, improving support for LSP clients that don't support pull diagnostics by doing the following:
didSavenotification type:didSavefor any document, we callpublish_diagnostics_if_neededon any open documents.Summary
Test Plan