Conversation
|
Diagnostics now properly show up like this: 
And clear out whenever the issue is fixed. |
|
Oh boy micha really merge conflicted me bad h'ok... |
| /// notification]. | ||
| /// | ||
| /// [publish diagnostics notification]: https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#textDocument_publishDiagnostics | ||
| pub(super) fn publish_settings_diagnostics(session: &mut Session, client: &Client) { |
There was a problem hiding this comment.
I wonder if we should change this method to take a &mut Project instead. It should be unlikely that a user changes all projects at once. It's more likely that they changed a setting for a specific project.
This would also ensure that we don't publish diagnostics for the default database (it should never have setting diagnostics but initializing it isn't entirely free)
There was a problem hiding this comment.
Huh, really? the default project really can't have any settings diagnostics?
There was a problem hiding this comment.
Also the current signature is "required" by the design of DidChangeWatchedFiles which processes random changes to random databases all together, producing only a single project_changed boolean. I could indeed make this more granular if we tracked project_changed per-project (but it's not clear to me it's worth the effort?).
There was a problem hiding this comment.
did_change_watched_files does group all files by database and then calls apply_changes exacltly once per database. See this loop where root maps to the root of a database:
This should allow you to make publish_settings_diagnostics per database.
Lol sorry. But not all of the merge conflicts are because of me. Some are because of Dhruv ;) |
ca1f1b4 to
84608c0
Compare
|
Rebased |
|
|
I'll do a full review tomorrow as it's late here. |
MichaReiser
left a comment
There was a problem hiding this comment.
This overall looks good.
We should remove the publish_settings_diagnostics calls in did_change and did_open because they're unnecessary. Instead, we should call publish_settings_diagnostics in Session::initialize_workspaces
I'd also recommend making publish_settings_diagnostics to take a specific project instead of iterating over all projects.
| /// notification]. | ||
| /// | ||
| /// [publish diagnostics notification]: https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#textDocument_publishDiagnostics | ||
| pub(super) fn publish_settings_diagnostics(session: &mut Session, client: &Client) { |
There was a problem hiding this comment.
did_change_watched_files does group all files by database and then calls apply_changes exacltly once per database. See this loop where root maps to the root of a database:
This should allow you to make publish_settings_diagnostics per database.
| } | ||
| } | ||
|
|
||
| publish_settings_diagnostics(session, client); |
There was a problem hiding this comment.
We should skip calling publish_settings_diagnostics if the client uses workspace diagnostics as it is unnecessary
There was a problem hiding this comment.
I moved the check inside the call so each callsite doesn't need to think about it.
| db, | ||
| untracked_files_with_pushed_diagnostics: untracked, | ||
| }, | ||
| ); |
There was a problem hiding this comment.
I think we need to call into publish_settings_diagnostics here to ensure the server pushes the diagnostics on initiale load.
caf9092 to
d4fbb20
Compare
|
Review addressed, should be ready to land. |
No description provided.