Feature: configure file size limit #668

JSteitz · 2018-08-29T21:13:25Z

Requires the PR from vscode-php-intellisense felixfbecker/vscode-php-intellisense#77
This PR is based on #308 and should be merged after

This solves issues / feature requests for #299 #548

Will take the options sent by the client. Option: php.intellisense.fileTypes = [".php"]

To sanitize the file type option, we provide a setter method for the property that will be called by the JsonMapper.

…into feature/allow-configurable-file-extension-for-indexing

Currently only the default Options type and the vscode format are accepted.

…e indexer

…figurable-file-extension-for-indexing

* merge latest upstream * remove currently not required code blocks * fix tests

JSteitz · 2018-08-29T21:14:57Z

src/Indexer.php

@@ -221,7 +221,7 @@ private function indexFiles(array $files): Promise
                yield timeout();
                $this->client->window->logMessage(MessageType::LOG, "Parsing $uri");
                try {
-                    $document = yield $this->documentLoader->load($uri);
+                    $document = yield $this->documentLoader->load($uri, $this->options->fileSizeLimit);


@felixfbecker I think this is the right place to pass the file size limit, what do you think?
Just asking, because I need to adjust a lot of tests for that.

felixfbecker · 2018-08-29T21:23:34Z

Please consider #308 (comment)

JSteitz · 2018-08-30T08:24:17Z

Do you mean the one comment with renaming the class Options to InitializationOptions only?
Or the whole conversation?

For the whole conversation I need to talk with you a bit more, because I have difficulties to understand why clients can't implement the initialization options. I understand why you want to move this to onDidChangeConfiguration, but I think that is the wrong place for this LanguageServer.

We can talk here or continue in #308.

felixfbecker · 2018-08-30T11:32:27Z

because I have difficulties to understand why clients can't implement the initialization options.

They can. But it's not defined in the protocol that initializationOptions contains configuration. So you basically have to tell every client "if you want this to work, you need to do this special thing that is not defined in the protocol". That is against the purpose of LSP - the client should not have to do special things per language server. And in this case, there is a proper way defined in the protocol to handle confiuration, so we should use that.

JSteitz · 2018-08-30T11:43:33Z

Alright, now I get it :) thx for the patience.

Here is my suggestion that should satisfy LSP:

We can hook into initialized and request configuration settings with workspace/configuration.
This also means that the indexer must be moved here. This shouldn't be a problem, since it is directly called after initialize. With this approach we can target only settings that will affect the indexer and use the didChangeConfiguration for other settings (e.g. Add validator frequency setting #302)

I really really don't want to start reindexing after every change in didChangeConfiguration.
We also can add a command to invalidate cache and reindex project.

felixfbecker · 2018-08-30T11:49:04Z

Yeah, using workspace/configuration sounds fine to me, I think that's actually a recent addition that I didn't hear about before.

I really really don't want to start reindexing after every change in didChangeConfiguration.

It would only have to reindex if a setting affecting the indexing changed, e.g. the file size limit. What is the problem with that?

JSteitz · 2018-08-30T11:57:08Z

Yes that is right, that it would only reindex affected settings.
See #308 (comment)

If we would move it to didChangeConfiguration then we need the ability to abort current running indexing (can we?) and restart with the new settings.
Personally I don't care if we can make it work like that.
If we can't than I don't want to index 2 times.

In the end, it's your decision :)

felixfbecker · 2018-08-30T12:09:03Z

We could pretty easily. Ideally we would pass some sort of CancellationSignal to coroutines. But a simple implementation can also just be letting the indexer have a private property that gets incremented with each run, and on every iteration, if the property changed, abort the current run.

JSteitz · 2018-08-30T12:19:48Z

Alright, which of these 2 Options should I implement.

initialized - reindexing after restart
didChangeConfiguration - reindexing without restart

felixfbecker · 2018-08-30T15:03:12Z

Both sounds like the best way - so we only start indexing after we know the initial configuration, but are still able to handle configuration changes (a didChangeConfiguration call without relevant changes should not trigger a reindex).

JSteitz · 2018-08-30T18:45:41Z

After testing with workspace/configuration and initialized I came to the conclusion, that we can not use that for now. It seems like it requires multiple workspaces to be active to work (capabilities for that are not set and a request results in "unhandled method ...").

So only the option for didChangeConfiguration is remaining.
Can you hint me how I can implement an CancelationSignal?
Otherwise I will do a simple method to cancel the indexing process.

felixfbecker · 2018-08-30T19:16:03Z

It seems like it requires multiple workspaces to be active to work (capabilities for that are not set and a request results in "unhandled method ...").

Could you explain this in more detail?

So only the option for didChangeConfiguration is remaining.
Can you hint me how I can implement an CancelationSignal?
Otherwise I will do a simple method to cancel the indexing process.

I would go with the simple method for now

JSteitz · 2018-08-30T19:56:37Z

Now I feel dump...
Thx for the help, after checking everything again so I don't tell bullshit I saw that the languageclient version was wrong for this feature.

Did an upgrade and now I see it :)
Will work on this later again.

felixfbecker · 2018-11-11T18:58:48Z

@JSteitz are still interested in getting this merged?

Jürgen Steitz and others added 29 commits February 18, 2017 01:38

Add support to index multiple file extensions

cdb5b56

Will take the options sent by the client. Option: php.intellisense.fileTypes = [".php"]

Add test for indexing multiple file types

5f096c4

Fix wrong phpDoc type

7dc4477

Filter invalid file types and use default list as fallback

f7175bc

Let JsonMapper intialize the options

9433694

To sanitize the file type option, we provide a setter method for the property that will be called by the JsonMapper.

Add test for fileTypes option

39cfbda

Initialize options with default values when not provided by client

3c33e7f

Update testIndexingMultipleFileTypes

d2e5048

Add missing namespace in OptionsTest

b9d0d1b

Fix wrong classname for options test

9067b44

Wipe index when on configuration change

1e319c7

Add list of valid indexer options

58c82e6

Implement didChangeConfiguration event

44a942e

Pass options and indexer to workspace

940eb97

Add tests

5b1b6bf

Merge branch 'master' of github.com:felixfbecker/php-language-server …

707c97f

…into feature/allow-configurable-file-extension-for-indexing

Improve gettting changed options

1e73d08

Update options one by one to update all instance

1f90b4e

Remove emitting wipe events

ca225ff

Accept different types/formats from clients

c4568bf

Currently only the default Options type and the vscode format are accepted.

Add new tests and update old ones

5308e7a

Fix phpcs warnings/errors

a06057b

Let didChangeConfiguration decide what options are interesting for th…

23a40f0

…e indexer

Change didChangeConfiguration doc to protocol wording

f4f1067

Merge remote-tracking branch 'upstream/master' into feature/allow-con…

9cc2736

…figurable-file-extension-for-indexing

Refactor pull request

09fbec2

* merge latest upstream * remove currently not required code blocks * fix tests

Fix risky test warning

a5417cd

Add option to set file size limit

24e3b77

Add basic test

b7bcf00

JSteitz commented Aug 29, 2018

View reviewed changes

JSteitz mentioned this pull request Aug 29, 2018

WIP: support intellisense options in vscode felixfbecker/vscode-php-intellisense#77

Open

4 tasks

felixfbecker added the work needed label Nov 11, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature: configure file size limit #668

Feature: configure file size limit #668

JSteitz commented Aug 29, 2018

JSteitz Aug 29, 2018

felixfbecker commented Aug 29, 2018

JSteitz commented Aug 30, 2018

felixfbecker commented Aug 30, 2018

JSteitz commented Aug 30, 2018

felixfbecker commented Aug 30, 2018

JSteitz commented Aug 30, 2018 •

edited

felixfbecker commented Aug 30, 2018

JSteitz commented Aug 30, 2018

felixfbecker commented Aug 30, 2018

JSteitz commented Aug 30, 2018

felixfbecker commented Aug 30, 2018

JSteitz commented Aug 30, 2018

felixfbecker commented Nov 11, 2018

Feature: configure file size limit #668

Are you sure you want to change the base?

Feature: configure file size limit #668

Conversation

JSteitz commented Aug 29, 2018

JSteitz Aug 29, 2018

Choose a reason for hiding this comment

felixfbecker commented Aug 29, 2018

JSteitz commented Aug 30, 2018

felixfbecker commented Aug 30, 2018

JSteitz commented Aug 30, 2018

felixfbecker commented Aug 30, 2018

JSteitz commented Aug 30, 2018 • edited

felixfbecker commented Aug 30, 2018

JSteitz commented Aug 30, 2018

felixfbecker commented Aug 30, 2018

JSteitz commented Aug 30, 2018

felixfbecker commented Aug 30, 2018

JSteitz commented Aug 30, 2018

felixfbecker commented Nov 11, 2018

JSteitz commented Aug 30, 2018 •

edited