-
Notifications
You must be signed in to change notification settings - Fork 185
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
base: master
Are you sure you want to change the base?
Allow configurable file extension for indexing #308
Conversation
Will take the options sent by the client. Option: php.intellisense.fileTypes = [".php"]
Codecov Report
@@ Coverage Diff @@
## master #308 +/- ##
============================================
+ Coverage 86.23% 86.28% +0.05%
- Complexity 718 729 +11
============================================
Files 53 54 +1
Lines 1460 1480 +20
============================================
+ Hits 1259 1277 +18
- Misses 201 203 +2
Continue to review full report at Codecov.
|
If everything is fine I will do the other 3 requests :) |
src/Indexer.php
Outdated
* @param Index $sourceIndex | ||
* @param PhpDocumentLoader $documentLoader | ||
* @param \stdClass|null $composerLock | ||
* @param IndexerOptions|null $options |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wrong type
src/Options.php
Outdated
{ | ||
// Do nothing when the $options parameter is not an object | ||
if (!is_object($options)) { | ||
if (!is_object($options) && !is_array($options) && (!$options instanceof \Traversable)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that won't work, you need to do it like this: !($options instanceof \Traversable)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll add tests for the Option class when I'm back
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jens1o actually instanceof
binds stronger than !
, so it works and the parenthesis around it are not needed. But I usually do them for readability.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah, thanks. Didn't knew that.
src/Options.php
Outdated
foreach ($options as $option => $value) { | ||
$method = 'set' . ucfirst($option); | ||
|
||
call_user_func([$this, $method], $value); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Check first wether the method exists?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thx will do it later when I'm back home again
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would rather not use setter methods at all and let Options
have public attributes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@felixfbecker How would you validate/filter the option? Calling them extra or using __set()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wouldn't. In the LS code I just don't assign invalid values and JSONMapper will throw if the @var
tags don't match the type
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will it notify the client about the invalid value type?
What about arrays that could have any type as value? E.g. $fileTypes = ['.php', false]
Since the options are coming from the client we should validate/filter them
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, the exception can be caught and propagated to the client.
What do you mean arrays that can have any type as value? $fileTypes
would be of type string[]
src/Options.php
Outdated
{ | ||
$fileTypes = filter_var_array($fileTypes, FILTER_SANITIZE_STRING); | ||
$fileTypes = filter_var($fileTypes, FILTER_CALLBACK, ['options' => [$this, 'filterFileTypes']]); | ||
$fileTypes = array_filter($fileTypes); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You need to provide a callback for array_filter
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With no callback it will remove all that are false, but I can add one
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://secure.php.net/manual/en/function.array-filter.php says, that the second parameter must be provided. But you could use strlen
, that will filter any false, empty strings or null.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Weird for me it says it is optional :/
If no callback is supplied, all entries of array equal to FALSE (see converting to boolean) will be removed.
public function setFileTypes(array $fileTypes) | ||
{ | ||
$fileTypes = filter_var_array($fileTypes, FILTER_SANITIZE_STRING); | ||
$fileTypes = filter_var($fileTypes, FILTER_CALLBACK, ['options' => [$this, 'filterFileTypes']]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it may be possible that you'll pass false into this method. Also, you've inserted a double space
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, did a manual test before I used it, will be tested later
src/LanguageServer.php
Outdated
* @return Promise <InitializeResult> | ||
*/ | ||
public function initialize(ClientCapabilities $capabilities, string $rootPath = null, int $processId = null): Promise | ||
public function initialize(ClientCapabilities $capabilities, string $rootPath = null, int $processId = null, $initializationOptions = null): Promise |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't know vscode sends the settings as initializationOptions
, I thought didChangeConfiguration
was the only way. Is there a reason you didn't type $initializationOptions
as a class (Options
)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Microsoft has a list with all methods and params: https://github.com/Microsoft/language-server-protocol/blob/master/versions/protocol-2-x.md#initialize
You only have to pass them from vscode as a property of ClientOptions
According to the Docs it can have any type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't force every client to do this. This needs to happen through onDidChangeConfiguration
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The actual options are taken from the configuration and are just passed with the initialize method. felixfbecker/vscode-php-intellisense@59e6f2c
So you want to move the indexing to onDidChangeConfiguration
? Otherwise you would index 2x, because this event is sent after the intialize (by vscode).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tricky. A client might never send onDidChangeConfiguration
. But it should definitely be retriggered if the config is sent through that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would open a new issue to discuss this part because it affects the indexing for many option combinations. And maybe delay it until we have implemented some of them.
For now it is okay, when the user has to reload to reindex with the changed options.
I have already some ideas but I need to collect more details for how the indexing will be affected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would want at least an implementation of onDidChangeConfiguration
that, if this setting changed, wipes the index and reindexes, because I don't want to add an important feature like this to the language server if it only works in VS Code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But feel free to discuss in an issue
src/Options.php
Outdated
$method = 'set' . ucfirst($option); | ||
|
||
call_user_func([$this, $method], $value); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would rather let JsonMapper
do this (like it does for all protocol structures) instead of doing this here
To sanitize the file type option, we provide a setter method for the property that will be called by the JsonMapper.
Codecov Report
@@ Coverage Diff @@
## master #308 +/- ##
===========================================
+ Coverage 84.15% 84.5% +0.34%
- Complexity 811 830 +19
===========================================
Files 59 60 +1
Lines 1717 1768 +51
===========================================
+ Hits 1445 1494 +49
- Misses 272 274 +2
Continue to review full report at Codecov.
|
This is a blocker for me, anything I can do here to speed it up? :) |
Code for the |
…into feature/allow-configurable-file-extension-for-indexing
Currently only the default Options type and the vscode format are accepted.
src/Server/Workspace.php
Outdated
{ | ||
if ($settings === null) { | ||
return; | ||
} | ||
|
||
// VSC sends the settings with the config section as main key | ||
if ($settings instanceof \stdClass && $settings->phpIntelliSense) { |
There was a problem hiding this comment.
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 ...
}
}
src/Server/Workspace.php
Outdated
// VSC sends the settings with the config section as main key | ||
if ($settings instanceof \stdClass && $settings->phpIntelliSense) { | ||
$mapper = new \JsonMapper(); | ||
$settings = $mapper->map($settings->phpIntelliSense, new Options); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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"
src/Server/Workspace.php
Outdated
$settings = $mapper->map($settings->phpIntelliSense, new Options); | ||
} | ||
|
||
if (!($settings instanceof Options)) { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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[]
src/Server/Workspace.php
Outdated
} | ||
|
||
if (!($settings instanceof Options)) { | ||
throw new \Exception('Settings format not valid.'); |
There was a problem hiding this comment.
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
src/Options.php
Outdated
* | ||
* @param array $fileTypes List of file types | ||
*/ | ||
public function setFileTypes(array $fileTypes) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These functions are not needed if you let JsonMapper take care of validation
src/Server/Workspace.php
Outdated
$this->options->$prop = $val; | ||
} | ||
|
||
if ($this->indexer && !empty(array_intersect($changedOptions, $indexerOptions))) { |
There was a problem hiding this comment.
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)
{ | ||
$sourceIndex = new Index; | ||
$dependenciesIndex = new DependenciesIndex; | ||
$projectIndex = $this->getMockBuilder('LanguageServer\Index\ProjectIndex') |
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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
src/Server/Workspace.php
Outdated
@@ -200,4 +214,75 @@ public function xdependencies(): array | |||
} | |||
return $dependencyReferences; | |||
} | |||
|
|||
/** | |||
* Fires when client changes settings in the client |
There was a problem hiding this comment.
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.
src/Server/Workspace.php
Outdated
* 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 |
There was a problem hiding this comment.
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?
src/LanguageServer.php
Outdated
@@ -190,6 +191,7 @@ public function initialize(ClientCapabilities $capabilities, string $rootPath = | |||
$this->projectIndex = new ProjectIndex($sourceIndex, $dependenciesIndex, $this->composerJson); | |||
$stubsIndex = StubsIndex::read(); | |||
$this->globalIndex = new GlobalIndex($stubsIndex, $this->projectIndex); | |||
$initializationOptions = $initializationOptions ?? new Options; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This way $initializationOptions
is not an instance of Options
if provided
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't get you here, is this comment a part from another comment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If $initializedOptions
is not null
, it will be stdClass
because it's not typed. I also think you don't account for the nesting under ->php
here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For $initializationOptions
you have more freedom how and which options will be send.
The $initializationOptions
parameter ist typed in initialize()
and is only passed as is to coroutine()
, so it should be enough only to check for null
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You pass this variable to the Indexer
further down, which accepts Options
, not stdClass
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm really sorry but I don't see a problem here.
$initializationOptions
is always of type Options
.
At which point will it be a stdClass
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh sorry, I think I missed the type annotation because I looked at the closure use
. But shouldn't this be the same object as passed to didChangeConfiguration
? And will this break passing custom properties from the client in initializationOptions
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
initializationOptions
can be completely different to didChangeConfiguration
.
And actually they are not the same in this PR. While initializationOptions
returns only defined options in the extension, didChangeConfiguration
will return the whole section with the section name as main key.
initializationOptions
{
"fileTypes": [
".php"
]
}
didChangeConfiguration
{
"php": {
"executablePath": "\/usr\/local\/opt\/php71\/bin\/php",
"fileTypes": [
".php"
],
"suggest": {
"basic": true
},
"validate": {
"enable": true,
"executablePath": "\/usr\/local\/opt\/php71\/bin\/php",
"run": "onSave"
}
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's up to us to defined - I am just saying it's a bit confusing that they are different. It would be much easier for a client if we just document please pass the didChangeConfiguration
settings as initializationOptions
src/Server/Workspace.php
Outdated
* Currently only the vscode format is supported | ||
* | ||
* @param mixed|null $settings | ||
* @return bool |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
src/Options.php
Outdated
public function setFileTypes(array $fileTypes) | ||
{ | ||
$fileTypes = filter_var_array($fileTypes, FILTER_SANITIZE_STRING); | ||
$fileTypes = filter_var($fileTypes, FILTER_CALLBACK, ['options' => [$this, 'filterFileTypes']]); // validate file type format |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is one space to much here.
any news on this? |
I dont think this will be done before #357 is done, and then it might be obsolete/require to be redone |
is somebody working on this? |
Currently no. Need to find some time to review and update to upstream. |
…figurable-file-extension-for-indexing
* merge latest upstream * remove currently not required code blocks * fix tests
After revisiting this PR I removed all code blocks that are not needed. My plans for now are:
|
Codecov Report
@@ Coverage Diff @@
## master #308 +/- ##
=============================================
- Coverage 81.52% 70.86% -10.66%
- Complexity 912 941 +29
=============================================
Files 62 63 +1
Lines 2078 2169 +91
=============================================
- Hits 1694 1537 -157
- Misses 384 632 +248
|
The indexer is moved to the method initialized, so we can request configurations from the client to init the indexer itself.
@felixfbecker I have difficulties with race conditions. Any hints how I can make the Expected order:
What actually happens:
|
Without fully understanding what's causing problems for you, you can save a |
Looks like I have a general problem with After investigating vscode-languageclient, I could see that the client is sending the reponse to my request after Both Do we drop this part and only implement |
* Handle the case where didChangeConfiguration is called before workspace/configuration request is resolved. * Implement basic cancellation signal request * Use defaults options and only apply new on request
Current state:
The cancel request for reindexing is working, but I'm not so happy with that. I was thinking to wrap the index in a try catch and throw an exception to abort indexing altogether, |
src/Client/Workspace.php
Outdated
* the result for the first configuration item in the params). | ||
* | ||
* @param ConfigurationItem[] $items | ||
* @return Promise |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the type of the Promise?
return $this->handler->request( | ||
'workspace/configuration', | ||
['items' => $items] | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be mapped to a class?
|
||
while ($this->isIndexing()) { | ||
yield timeout(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not that happy with this "polling" solution - usually cancellation is synchronous and best-effort. I.e. the old indexing would stop eventually but you can already start with the next index run, by just making sure to instantiate new Index objects (maybe even a new Indexer) so that the old run can't accidentally still write to the new index. Does that sound feasible?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't think a new Indexer instance is a good idea, they are used in 2 different places (initialized
and didChangeConfiguration
) and I would need a reference to both.
And I think creating new Index objects is too much work from what I see.
Are you talking about sourceIndex
and dependencyIndex
?
If yes, then the method would need to know about all objects that require the index objects. Or am I wrong here?
src/Protocol/ConfigurationItem.php
Outdated
|
||
namespace LanguageServer\Protocol; | ||
|
||
class ConfigurationItem |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A constructor would be good to reduce boilerplate
src/LanguageServer.php
Outdated
$configurationitem = new ConfigurationItem(); | ||
$configurationitem->section = 'php'; | ||
$configuration = yield $this->client->workspace->configuration([$configurationitem]); | ||
$options = $this->mapper->map($configuration[0], new Options()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be done by the configuration
method?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As long as we don't say, we will only use settings from the section "php", i would not do that, because I know that people will ask for including files.exclude
setting (see #159 (comment)).
// the workspace/didChangeConfiguration can be invoked before | ||
// the response from the workspace/configuration request is resolved | ||
if ($this->indexer->isIndexing()) { | ||
return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And why do you need to return in that case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because when the settings are different from default, the didChangeConfiguration
is initiating the indexer. When it is already running, we know the running indexer is using the latest settings.
$this->indexer->setOptions($options); | ||
} | ||
|
||
$this->indexer->index()->otherwise('\\LanguageServer\\crash'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should yield
this instead and make the whole coroutine crash on error since notifications can't return error responses
Requires the PR from vscode-php-intellisense felixfbecker/vscode-php-intellisense#77
Fixes #145 #300