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

Allow configurable file extension for indexing #308

Open
wants to merge 31 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
31 commits
Select commit Hold shift + click to select a range
cdb5b56
Add support to index multiple file extensions
Feb 18, 2017
5f096c4
Add test for indexing multiple file types
Feb 18, 2017
7dc4477
Fix wrong phpDoc type
Feb 18, 2017
f7175bc
Filter invalid file types and use default list as fallback
Feb 18, 2017
9433694
Let JsonMapper intialize the options
Feb 18, 2017
39cfbda
Add test for fileTypes option
Feb 18, 2017
3c33e7f
Initialize options with default values when not provided by client
Feb 18, 2017
d2e5048
Update testIndexingMultipleFileTypes
Feb 18, 2017
b9d0d1b
Add missing namespace in OptionsTest
Feb 18, 2017
9067b44
Fix wrong classname for options test
Feb 18, 2017
1e319c7
Wipe index when on configuration change
Feb 24, 2017
58c82e6
Add list of valid indexer options
Mar 2, 2017
44a942e
Implement didChangeConfiguration event
Mar 2, 2017
940eb97
Pass options and indexer to workspace
Mar 2, 2017
5b1b6bf
Add tests
Mar 2, 2017
707c97f
Merge branch 'master' of github.com:felixfbecker/php-language-server …
Mar 2, 2017
1e73d08
Improve gettting changed options
Mar 4, 2017
1f90b4e
Update options one by one to update all instance
Mar 4, 2017
ca225ff
Remove emitting wipe events
Mar 4, 2017
c4568bf
Accept different types/formats from clients
Mar 4, 2017
5308e7a
Add new tests and update old ones
Mar 4, 2017
a06057b
Fix phpcs warnings/errors
Mar 4, 2017
23a40f0
Let didChangeConfiguration decide what options are interesting for th…
Mar 4, 2017
f4f1067
Change didChangeConfiguration doc to protocol wording
Mar 4, 2017
9cc2736
Merge remote-tracking branch 'upstream/master' into feature/allow-con…
JSteitz Aug 29, 2018
09fbec2
Refactor pull request
JSteitz Aug 29, 2018
a5417cd
Fix risky test warning
JSteitz Aug 29, 2018
e317e8c
Start indexing after initialization
JSteitz Aug 31, 2018
a1c3845
WIP: Implement didChangeConfiguration with reindexing
JSteitz Aug 31, 2018
a1e5654
WIP: Implement didChangeConfiguration with reindexing
JSteitz Aug 31, 2018
a81bed9
Partial work on feedback
JSteitz Aug 31, 2018
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 0 additions & 3 deletions src/Index/AbstractAggregateIndex.php
Original file line number Diff line number Diff line change
Expand Up @@ -42,9 +42,6 @@ protected function registerIndex(ReadableIndex $index)
$index->on('definition-added', function () {
$this->emit('definition-added');
});
$index->on('wipe', function() {
$this->emit('wipe');
});
}

/**
Expand Down
2 changes: 0 additions & 2 deletions src/Index/Index.php
Original file line number Diff line number Diff line change
Expand Up @@ -218,7 +218,5 @@ public function wipe()
$this->references = [];
$this->complete = false;
$this->staticComplete = false;

$this->emit('wipe');
}
}
15 changes: 0 additions & 15 deletions src/Options.php
Original file line number Diff line number Diff line change
Expand Up @@ -11,11 +11,6 @@ class Options
*/
public $fileTypes = ['.php'];

/**
* List of options that affect the indexer
*/
private $indexerOptions = ['fileTypes'];

/**
* Validate/Filter input and set options for file types
*
Expand All @@ -31,16 +26,6 @@ public function setFileTypes(array $fileTypes)
$this->fileTypes = !empty($fileTypes) ? $fileTypes : $this->fileTypes;
}

/**
* Get list with options that affect the indexer
*
* @return array
*/
public function getIndexerOptions(): array
{
return $this->indexerOptions;
}

/**
* Filter valid file type
*
Expand Down
61 changes: 46 additions & 15 deletions src/Server/Workspace.php
Original file line number Diff line number Diff line change
Expand Up @@ -216,28 +216,55 @@ public function xdependencies(): array
}

/**
* @param Options|null $settings
* Fires when client changes settings in the client
Copy link
Owner

Choose a reason for hiding this comment

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

Change this to the protocol wording:

A notification sent from the client to the server to signal the change of configuration settings.

https://github.com/Microsoft/language-server-protocol/blob/master/protocol.md#didchangeconfiguration-notification

*
* The default paramter type is Options but it also accepts different types
* which will be transformed on demand.
*
* Currently only the vscode format is supported
Copy link
Owner

Choose a reason for hiding this comment

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

What does this mean? Where is the problem in saying the PHP LS reads settings under the php key?

*
* @param mixed|null $settings
* @return bool
Copy link
Owner

Choose a reason for hiding this comment

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

what is the return value for? A notification does not return anything

Copy link
Author

Choose a reason for hiding this comment

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

I know and I was hesitating to add the return value, but for some tests I need them.
When you have another idea how I can test them, tell me.

see for example testNoOptionPassed or testNoChangedOptions

Copy link
Owner

Choose a reason for hiding this comment

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

You shouldn't add return values just to do assertions when what you really want to test is that certain side effects happen. You should mock classes and then assert that a method is called or not.

* @throws \Exception Settings format not valid
*/
public function didChangeConfiguration(Options $settings = null)
public function didChangeConfiguration($settings = null): bool
{
// List of options that affect the indexer
$indexerOptions = ['fileTypes'];

if ($settings === null) {
return;
return false;
}

// VSC sends the settings with the config section as main key
if ($settings instanceof \stdClass && $settings->phpIntelliSense) {
Copy link
Author

Choose a reason for hiding this comment

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

Not happy with this part.
The documentation is lacking in which format the settings are passed to the server and why they do it that way.
With all the different tests I did, they always send it like that:

{
    "ExtensionConfigSectionName": { // defined in the package.json in the vscode extensions
        ... the actual settings ...
    }
}

$mapper = new \JsonMapper();
$settings = $mapper->map($settings->phpIntelliSense, new Options);
Copy link
Owner

Choose a reason for hiding this comment

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

Can we just use php as the config section? "IntelliSense" is a term only associated with VS and VS Code.

Copy link
Author

Choose a reason for hiding this comment

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

Sure we can.

We only have to lookout for possible conflicts in the future when we use php as config section.

Possible config naming:

  • php.indexer.*
  • php.completion.* (possible name for a feature request, but can be ignored now)
  • php.intellisense.* (previous name)

Copy link
Owner

Choose a reason for hiding this comment

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

php.fileExtensions should be fine. I don't think we should concern user with the concept of an "indexer"

}

if (!($settings instanceof Options)) {
Copy link
Owner

Choose a reason for hiding this comment

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

Afaik JsonMapper will throw when the schema doesn't match

Copy link
Author

Choose a reason for hiding this comment

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

Do you have to tell him to that?
Because for me JsonMapper never threw an exception when the schema didn't match.
It silently ignored invalid properties and used the defaults in the Options.

Copy link
Owner

Choose a reason for hiding this comment

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

What case exactly did not throw an exception? It could be because you typed setFileTypes() as taking array, not string[]

throw new \Exception('Settings format not valid.');
Copy link
Owner

Choose a reason for hiding this comment

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

This should probably cause a window/showMessage since notifications do not return a response

}

$changedOptions = $this->getChangedOptions($settings);
$this->options = $settings;

if (!empty(array_intersect($changedOptions, $this->options->getIndexerOptions()))) {
if (empty($changedOptions)) {
return false;
}

foreach (get_object_vars($settings) as $prop => $val) {
$this->options->$prop = $val;
}

if ($this->indexer && !empty(array_intersect($changedOptions, $indexerOptions))) {
Copy link
Owner

Choose a reason for hiding this comment

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

Can we for now just keep it simple and instead of getChangedOptions() just do

if ($this->options->fileTypes != $settings->php->fileTypes)

// check list of options that changed since last time against the list of valid indexer options

// start wiping from the main index
// wipe main index and start reindexing
$this->index->wipe();

// check for existing indexer and start indexing
if ($this->indexer) {
$this->indexer->index()->otherwise('\\LanguageServer\\crash');
}
$this->indexer->index()->otherwise('\\LanguageServer\\crash');
}

return true;
}

/**
Expand All @@ -248,10 +275,14 @@ public function didChangeConfiguration(Options $settings = null)
*/
private function getChangedOptions(Options $settings): array
{
// squash nested array for comparing changed options
$old = array_map('json_encode', get_object_vars($this->options));
$new = array_map('json_encode', get_object_vars($settings));
$old = get_object_vars($this->options);
$new = get_object_vars($settings);
$changed = array_udiff($old, $new, function ($a, $b) {
// custom callback since array_diff uses strings for comparison

return $a <=> $b;
});

return array_keys(array_diff($old, $new));
return array_keys($changed);
}
}
21 changes: 7 additions & 14 deletions tests/Server/ServerTestCase.php
Original file line number Diff line number Diff line change
Expand Up @@ -31,10 +31,6 @@ abstract class ServerTestCase extends TestCase
*/
protected $documentLoader;

protected $projectIndex;
protected $input;
protected $output;

/**
* Map from FQN to Location of definition
*
Expand All @@ -53,23 +49,20 @@ public function setUp()
{
$sourceIndex = new Index;
$dependenciesIndex = new DependenciesIndex;
$this->projectIndex = new ProjectIndex($sourceIndex, $dependenciesIndex);
$this->projectIndex->setComplete();
$projectIndex = new ProjectIndex($sourceIndex, $dependenciesIndex);
$projectIndex->setComplete();

$rootPath = realpath(__DIR__ . '/../../fixtures/');
$options = new Options;
$filesFinder = new FileSystemFilesFinder;
$cache = new FileSystemCache;

$this->input = new MockProtocolStream;
$this->output = new MockProtocolStream;

$definitionResolver = new DefinitionResolver($this->projectIndex);
$client = new LanguageClient($this->input, $this->output);
$this->documentLoader = new PhpDocumentLoader(new FileSystemContentRetriever, $this->projectIndex, $definitionResolver);
$this->textDocument = new Server\TextDocument($this->documentLoader, $definitionResolver, $client, $this->projectIndex);
$definitionResolver = new DefinitionResolver($projectIndex);
$client = new LanguageClient(new MockProtocolStream, new MockProtocolStream);
$this->documentLoader = new PhpDocumentLoader(new FileSystemContentRetriever, $projectIndex, $definitionResolver);
$this->textDocument = new Server\TextDocument($this->documentLoader, $definitionResolver, $client, $projectIndex);
$indexer = new Indexer($filesFinder, $rootPath, $client, $cache, $dependenciesIndex, $sourceIndex, $this->documentLoader, null, null, $options);
$this->workspace = new Server\Workspace($client, $this->projectIndex, $dependenciesIndex, $sourceIndex, null, $this->documentLoader, null, $indexer, $options);
$this->workspace = new Server\Workspace($client, $projectIndex, $dependenciesIndex, $sourceIndex, null, $this->documentLoader, null, $indexer, $options);

$globalSymbolsUri = pathToUri(realpath(__DIR__ . '/../../fixtures/global_symbols.php'));
$globalReferencesUri = pathToUri(realpath(__DIR__ . '/../../fixtures/global_references.php'));
Expand Down
145 changes: 113 additions & 32 deletions tests/Server/Workspace/DidChangeConfigurationTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -3,55 +3,139 @@

namespace LanguageServer\Tests\Server\Workspace;

use LanguageServer\Tests\MockProtocolStream;
use LanguageServer\Tests\Server\ServerTestCase;
use LanguageServer\{Server, Client, LanguageClient, Project, PhpDocument, Options};
use LanguageServer\Protocol\{
Message,
MessageType,
TextDocumentItem,
TextDocumentIdentifier,
SymbolInformation,
SymbolKind,
DiagnosticSeverity,
FormattingOptions,
Location,
Range,
Position
};
use AdvancedJsonRpc\{Request as RequestBody, Response as ResponseBody};
use function LanguageServer\pathToUri;
use LanguageServer\Tests\MockProtocolStream;
use LanguageServer\{Server, LanguageClient, PhpDocumentLoader, DefinitionResolver, Options, Indexer};
use LanguageServer\Index\{ProjectIndex, StubsIndex, GlobalIndex, DependenciesIndex, Index};
use LanguageServer\ContentRetriever\FileSystemContentRetriever;
use LanguageServer\Protocol\{Position, Location, Range, ClientCapabilities, Message, MessageType};
use LanguageServer\FilesFinder\FileSystemFilesFinder;
use LanguageServer\Cache\FileSystemCache;
use LanguageServer\Server\Workspace;
use Sabre\Event\Promise;
use Exception;

class DidChangeConfigurationTest extends ServerTestCase
{
public function testWipingIndex()
/**
* didChangeConfiguration does not need to do anything when no options/settings are passed
*/
public function testNoOptionPassed()
{
$promise = new Promise;
$client = new LanguageClient(new MockProtocolStream(), $writer = new MockProtocolStream());
$projectIndex = new ProjectIndex($sourceIndex = new Index(), $dependenciesIndex = new DependenciesIndex());
$definitionResolver = new DefinitionResolver($projectIndex);
$loader = new PhpDocumentLoader(new FileSystemContentRetriever(), $projectIndex, $definitionResolver);
$workspace = new Server\Workspace($client, $projectIndex, $dependenciesIndex, $sourceIndex, null, $loader, null);

$result = $workspace->didChangeConfiguration();
$this->assertFalse($result);
}

/**
* When the passed options/settings do not differ from the previous, it has nothing to do
*/
public function testFailsWithInvalidOptionsTypeOrFormat()
{
$options = new Options;
$client = new LanguageClient(new MockProtocolStream(), $writer = new MockProtocolStream());
$projectIndex = new ProjectIndex($sourceIndex = new Index(), $dependenciesIndex = new DependenciesIndex());
$definitionResolver = new DefinitionResolver($projectIndex);
$loader = new PhpDocumentLoader(new FileSystemContentRetriever(), $projectIndex, $definitionResolver);
$workspace = new Server\Workspace($client, $projectIndex, $dependenciesIndex, $sourceIndex, null, $loader, null, null, null, null, $options);

$this->expectException(\Exception::class);
$this->workspace->didChangeConfiguration(['invalid' => 'options format']);
}

/**
* When the passed options/settings do not differ from the previous, it has nothing to do
*/
public function testNoChangedOptions()
{
$options = new Options;
$client = new LanguageClient(new MockProtocolStream(), $writer = new MockProtocolStream());
$projectIndex = new ProjectIndex($sourceIndex = new Index(), $dependenciesIndex = new DependenciesIndex());
$definitionResolver = new DefinitionResolver($projectIndex);
$loader = new PhpDocumentLoader(new FileSystemContentRetriever(), $projectIndex, $definitionResolver);
$workspace = new Server\Workspace($client, $projectIndex, $dependenciesIndex, $sourceIndex, null, $loader, null, null, null, null, $options);

$result = $this->workspace->didChangeConfiguration($options);
$this->assertFalse($result);
}

/**
* Verify that the required methods for a reindex are called
*/
public function testFileTypesOptionTriggersAReindex()
{
$sourceIndex = new Index;
$dependenciesIndex = new DependenciesIndex;
$projectIndex = $this->getMockBuilder('LanguageServer\Index\ProjectIndex')
Copy link
Owner

Choose a reason for hiding this comment

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

Is using ::class not possible?

Awesome you figured out mocking, this will be a much better pattern for asserting client interaction in the future than using event handlers on the protocol reader!

Copy link
Author

Choose a reason for hiding this comment

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

Ohh yeah my bad, forgot that we have ::class

->setConstructorArgs([$sourceIndex, $dependenciesIndex])
->setMethods(['wipe'])
->getMock();
$projectIndex->setComplete();

$rootPath = realpath(__DIR__ . '/../../../fixtures/');
$filesFinder = new FileSystemFilesFinder;
$cache = new FileSystemCache;

$definitionResolver = new DefinitionResolver($projectIndex);
$client = new LanguageClient(new MockProtocolStream, new MockProtocolStream);
$documentLoader = new PhpDocumentLoader(new FileSystemContentRetriever, $projectIndex, $definitionResolver);
$textDocument = new Server\TextDocument($documentLoader, $definitionResolver, $client, $projectIndex);
$indexer = $this->getMockBuilder('LanguageServer\Indexer')
->setConstructorArgs([$filesFinder, $rootPath, $client, $cache, $dependenciesIndex, $sourceIndex, $documentLoader, null, null, new Options])
->setMethods(['index'])
->getMock();
$workspace = new Server\Workspace($client, $projectIndex, $dependenciesIndex, $sourceIndex, null, $documentLoader, null, $indexer, new Options);

$this->projectIndex->on('wipe', function() use ($promise) {
$promise->fulfill();
});

$options = new Options;
$options->fileTypes = [
'.inc'
];

$this->workspace->didChangeConfiguration($options);
$promise->wait();
$projectIndex->expects($this->once())->method('wipe');
$indexer->expects($this->once())->method('index');

// invoke event
$result = $workspace->didChangeConfiguration($options);
$this->assertTrue($result);
}

public function testReindexingAfterWipe()
/**
* Be sure that the indexer gets the new options/settings and uses them
*/
public function testIndexerUsesNewOptions()
{
$promise = new Promise;
$sourceIndex = new Index;
$dependenciesIndex = new DependenciesIndex;
$projectIndex = new ProjectIndex($sourceIndex, $dependenciesIndex);
$projectIndex->setComplete();

$rootPath = realpath(__DIR__ . '/../../../fixtures/');
$filesFinder = new FileSystemFilesFinder;
$cache = new FileSystemCache;
$initialOptions = new Options;

$input = new MockProtocolStream;
$output = new MockProtocolStream;

$definitionResolver = new DefinitionResolver($projectIndex);
$client = new LanguageClient($input, $output);
$documentLoader = new PhpDocumentLoader(new FileSystemContentRetriever, $projectIndex, $definitionResolver);
$textDocument = new Server\TextDocument($documentLoader, $definitionResolver, $client, $projectIndex);
$indexer = new Indexer($filesFinder, $rootPath, $client, $cache, $dependenciesIndex, $sourceIndex, $documentLoader, null, null, $initialOptions);
$workspace = new Server\Workspace($client, $projectIndex, $dependenciesIndex, $sourceIndex, null, $documentLoader, null, $indexer, $initialOptions);

$this->output->on('message', function (Message $msg) use ($promise) {
$output->on('message', function (Message $msg) use ($promise) {
if ($msg->body->method === 'window/logMessage' && $promise->state === Promise::PENDING) {
if ($msg->body->params->type === MessageType::ERROR) {
$promise->reject(new Exception($msg->body->params->message));
} elseif (strpos($msg->body->params->message, 'All 0 PHP files parsed') !== false) {
} elseif (strpos($msg->body->params->message, 'All 1 PHP files parsed') !== false) {
$promise->fulfill();
}
}
Expand All @@ -62,11 +146,8 @@ public function testReindexingAfterWipe()
'.inc'
];

$this->workspace->didChangeConfiguration($options);
$result = $workspace->didChangeConfiguration($options);
$this->assertTrue($result);
$promise->wait();
}

public function testGetChangedOptions()
{
}
}