Skip to content

Conversation

@TaKO8Ki
Copy link
Contributor

@TaKO8Ki TaKO8Ki commented Nov 1, 2025

Summary

Fixes #20784 cc: @MichaReiser

This is a follow up to #21105.

#21105 affected Notebook, so in this pull request I just fixed clear_diagnostics_for_document that is not related to Notebook.

Test Plan

output2

Comment on lines +46 to +48
if session.resolved_client_capabilities().pull_diagnostics {
return Ok(());
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change only affects DidClose. For Notebook, there is DidCloseNotebook, so it should not affect Notebook.

impl super::SyncNotificationHandler for DidClose {

impl super::SyncNotificationHandler for DidCloseNotebook {

@github-actions
Copy link
Contributor

github-actions bot commented Nov 1, 2025

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

return Ok(());
};
clear_diagnostics_for_document(snapshot.query(), client)?;
clear_diagnostics_for_document(session, snapshot.query(), client)?;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure this is correct. This is probably a bit surprising but notebook cells are text documents and the LSP sends close document requests for LSP cells.

Would you mind extending your test plan to cover

  • Closing a notebook that has diagnostics
  • Removing a cell that contains diagnostics

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

clear_diagnostics sends a publishDiagnostics notification without checking the client capabilities

2 participants