Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Changes from 20 commits
cdb5b56
5f096c4
7dc4477
f7175bc
9433694
39cfbda
3c33e7f
d2e5048
b9d0d1b
9067b44
1e319c7
58c82e6
44a942e
940eb97
5b1b6bf
707c97f
1e73d08
1f90b4e
ca225ff
c4568bf
5308e7a
a06057b
23a40f0
f4f1067
9cc2736
09fbec2
a5417cd
e317e8c
a1c3845
a1e5654
a81bed9
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 ofOptions
if providedThere 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 notnull
, it will bestdClass
because it's not typed. I also think you don't account for the nesting under->php
hereThere 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 ininitialize()
and is only passed as is tocoroutine()
, so it should be enough only to check fornull
.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 acceptsOptions
, notstdClass
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 typeOptions
.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 todidChangeConfiguration
? And will this break passing custom properties from the client ininitializationOptions
?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 todidChangeConfiguration
.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
didChangeConfiguration
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 asinitializationOptions
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
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.
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 like to keep
Options
a dumb data object (like other protocol structures).Checking if reindexing is needed should be done solely in
didChangeConfiguration
.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:
https://github.com/Microsoft/language-server-protocol/blob/master/protocol.md#didchangeconfiguration-notification
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?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:
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"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 takingarray
, notstring[]