Skip to content
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

Open
wants to merge 31 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 2 commits
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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/Indexer.php
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ class Indexer
* @param Index $sourceIndex
* @param PhpDocumentLoader $documentLoader
* @param \stdClass|null $composerLock
* @param IndexerOptions|null $options
* @param Options|null $options
*/
public function __construct(
FilesFinder $filesFinder,
Expand Down Expand Up @@ -112,7 +112,7 @@ public function __construct(
public function index(): Promise
{
return coroutine(function () {
$fileTypes = implode(',', $this->options->fileTypes);
$fileTypes = implode(',', $this->options->getFileTypes());
$pattern = Path::makeAbsolute('**/*{' . $fileTypes . '}', $this->rootPath);
felixfbecker marked this conversation as resolved.
Show resolved Hide resolved
$uris = yield $this->filesFinder->find($pattern);

Expand Down
60 changes: 49 additions & 11 deletions src/Options.php
Original file line number Diff line number Diff line change
Expand Up @@ -9,29 +9,67 @@ class Options
*
* @var array
*/
public $fileTypes = [".php"];
private $fileTypes = [".php"];

/**
* @param \stdClass|null $options
* @param \Traversable|\stdClass|array|null $options
*/
public function __construct(\stdClass $options = null)
public function __construct($options = null)
{
// Do nothing when the $options parameter is not an object
if (!is_object($options)) {
if (!is_object($options) && !is_array($options) && (!$options instanceof \Traversable)) {
Copy link
Contributor

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)

Copy link
Author

@JSteitz JSteitz Feb 18, 2017

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

Copy link
Owner

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.

Copy link
Contributor

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.

return;
}

$this->fileTypes = $options->fileTypes ?? $this->normalizeFileTypes($this->fileTypes);
foreach ($options as $option => $value) {
$method = 'set' . ucfirst($option);

call_user_func([$this, $method], $value);
Copy link
Contributor

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?

Copy link
Author

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

Copy link
Owner

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.

Copy link
Author

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()?

Copy link
Owner

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

Copy link
Author

@JSteitz JSteitz Feb 18, 2017

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

Copy link
Owner

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

}
Copy link
Owner

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

}

/**
* Validate and set options for file types
*
* @param array $fileTypes List of file types
*/
public function setFileTypes(array $fileTypes)
Copy link
Owner

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

{
$fileTypes = filter_var_array($fileTypes, FILTER_SANITIZE_STRING);
$fileTypes = filter_var($fileTypes, FILTER_CALLBACK, ['options' => [$this, 'filterFileTypes']]);
Copy link
Contributor

@jens1o jens1o Feb 18, 2017

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

Copy link
Author

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

$fileTypes = array_filter($fileTypes);
Copy link
Contributor

@jens1o jens1o Feb 18, 2017

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

Copy link
Author

@JSteitz JSteitz Feb 18, 2017

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

Copy link
Contributor

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.

Copy link
Author

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.


$this->fileTypes = !empty($fileTypes) ? $fileTypes : $this->fileTypes;
}

/**
* Get list of registered file types
*
* @return array
*/
public function getFileTypes(): array
{
return $this->fileTypes;
}

private function normalizeFileTypes(array $fileTypes): array
/**
* Filter valid file type
*
* @param string $fileType The file type to filter
* @return string|bool If valid it returns the file type, otherwise false
*/
private function filterFileTypes(string $fileType)
{
return array_map(function (string $fileType) {
if (substr($fileType, 0, 1) !== '.') {
$fileType = '.' . $fileType;
}
$fileType = trim($fileType);

if (empty($fileType)) {
return $fileType;
}, $fileTypes);
}

if (substr($fileType, 0, 1) !== '.') {
return false;
}

return $fileType;
}
}