Skip to content

Allow configurable file extension for indexing #308

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

Open
wants to merge 31 commits into
base: master
Choose a base branch
from
Open
Changes from 1 commit
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
26 changes: 24 additions & 2 deletions src/Server/Workspace.php
Original file line number Diff line number Diff line change
Expand Up @@ -216,16 +216,38 @@ 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 void
*/
public function didChangeConfiguration(Options $settings = null)
public function didChangeConfiguration($settings = null)
{
if ($settings === null) {
return;
}

// 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[]

return;
}

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

if (empty($changedOptions)) {
return;
}

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