Skip to content
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

Ensure diagnostics are cleared on file deletion #319

Merged
merged 3 commits into from
Mar 1, 2017

Conversation

mousetraps
Copy link
Contributor

@mousetraps mousetraps commented Mar 1, 2017

Previously, error diagnostics would not be cleared when a file was deleted while it was closed. This would result in lingering errors in the problems view that could only be cleared by reloading the language server. This fix addresses the issue by adding support for workspace/didChangeWatchedFiles and automatically clearing diagnostics for deleted files.

Additional Notes / Questions

  • can add some more tests if it turns out that this is the correct approach, but wanted to get it out there for early feedback.
  • Is there a better way to test this? e.g. Currently the test is not resilient to the method name not matching the protocol name (test would succeed, but scenario would fail in real life)
  • I discovered another couple issues while working through this which would be much easier to address after this fix:
    • language server only pulls in files during initialize, which means that files added from outside of the current editor instance are not being picked up.
    • language server does not properly update definitions for deleted, changed, or moved files that are not opened in the editor.
  • the change also requires a fix to vscode-php-intellisense to enable fileEvents in clientOptions. If there are options to include non *.php files in language server completions, this should be considered.
  • I'm fairly certain PHP 7 optimizes empty array initializations, but need to double check on this.

mousetraps and others added 2 commits March 1, 2017 00:54
Previously, error diagnostics would not be cleared when a file was deleted while it was closed. This would result in lingering errors in the problems view that could only be cleared by reloading the language server. This fix addresses the issue by adding support for workspace/didChangeWatchedFiles and automatically clearing diagnostics for deleted files.
@codecov
Copy link

codecov bot commented Mar 1, 2017

Codecov Report

Merging #319 into master will decrease coverage by -1.83%.
The diff coverage is 100%.

@@             Coverage Diff              @@
##             master     #319      +/-   ##
============================================
- Coverage     85.98%   84.15%   -1.83%     
- Complexity      717      811      +94     
============================================
  Files            53       59       +6     
  Lines          1463     1717     +254     
============================================
+ Hits           1258     1445     +187     
- Misses          205      272      +67
Impacted Files Coverage Δ Complexity Δ
src/Protocol/FileEvent.php 100% <100%> (ø) 1 <1> (?)
src/LanguageServer.php 92.77% <100%> (ø) 20 <0> (?)
src/Server/Workspace.php 68.96% <100%> (-19.27%) 30 <4> (+6)
src/ComposerScripts.php 0% <0%> (ø) 10% <0%> (+10%)
...rc/ContentRetriever/FileSystemContentRetriever.php 100% <0%> (ø) 1% <0%> (?)
src/Index/ProjectIndex.php 100% <0%> (ø) 4% <0%> (?)
src/PhpDocumentLoader.php 100% <0%> (ø) 12% <0%> (?)
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 56bd465...fc19640. Read the comment docs.

Copy link
Owner

@felixfbecker felixfbecker left a comment

Choose a reason for hiding this comment

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

Thanks! Implementing didChangeWatchedFiles was on my todo for a long time.
Especially adding or changing a file should cause a (re)index of the file, for example on a branch switch.

Regarding the tests, I think the best would be to use a mocking library like prophecy to mock the LanguageClient and assert that publishDiagnostics was called. I tried that once but failed to get it working so I just went with listening for the message event in the other tests, which works.

Just the code style change to merge this


$fileEvent = new FileEvent();
$fileEvent->uri = 'my uri';
$fileEvent->type = FileChangeType::DELETED;
Copy link
Owner

Choose a reason for hiding this comment

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

I think it makes sense to add a constructor to FileEvent to make this easier to construct

$fileEvent->type = FileChangeType::DELETED;

$isDiagnosticsCleared = false;
$writer->on('message', function (Message $message) use ($fileEvent, & $isDiagnosticsCleared) {
Copy link
Owner

Choose a reason for hiding this comment

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

No space after &

@mousetraps
Copy link
Contributor Author

Done, thanks for the feedback! Also made the corresponding PR at felixfbecker/vscode-php-intellisense#83. And with that, I'm off to bed because it's 2am here. 💤

@felixfbecker felixfbecker merged commit 0de7ba8 into felixfbecker:master Mar 1, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants