-
Notifications
You must be signed in to change notification settings - Fork 187
Feature: configure file size limit #668
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
Feature: configure file size limit #668
Conversation
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.
...figurable-file-extension-for-indexing
* merge latest upstream * remove currently not required code blocks * fix tests
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 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.
Please consider #308 (comment)
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.
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.
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.
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?
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 :)
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.
Alright, which of these 2 Options should I implement.
initialized- reindexing after restartdidChangeConfiguration- reindexing without restart
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).
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.
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
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.
@JSteitz are still interested in getting this merged?
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