phpactor / phpactor

Mainly a PHP Language Server with more features than you can shake a stick at
MIT License
1.44k stars 131 forks source link

Should use imports take parts of namespace in account? #742

Closed Einenlum closed 3 months ago

Einenlum commented 5 years ago

Here is a suggestion of improvement that would help me in my everyday work (so I guess maybe it could help others).

Current behavior

For now, the vim plugin only uses the word under the cursor to use classes. This works for most cases but I think sometimes it could have a better behavior. For people (like me) who sometimes use a part of the namespace in the typehint, here is what happens:

<?php

namespace App\Foo\Bar;

class Baz
{
    public __construct(
        Finder\User $userFinder,
        Finder\Place $placeFinder
    ) {
        $this->userFinder = $userFinder;
        $this->placeFinder = $placeFinder;
    }
}

If my cursor is on Place, I will have the following Results:

Select class:
1) App\Infrastructure\Serializer\Factory\Place
2) App\Domain\Finder\Place
3) App\Infrastructure\Doctrine\Repository\Place
4) App\Domain\Model\Place
5) App\Infrastructure\Serializer\Proxy\Place
6) App\Domain\Repository\Place

Although it is clear that the choice number 2 is the only possible answer, I get all these different results. Moreover, if I select the choice 2, App\Domain\Finder\Place will be added in my use statements. The expected use statement would be App\Domain\Finder though.

On the other hand, if my cursor is on Finder (although the full typehint I am on is Finder\Place), I get the following results:

Select class:
1) Symfony\Component\Finder\Finder
2) Nette\Utils\Finder
3) PhpCsFixer\Finder
4) Doctrine\Migrations\Finder\Finder

None of them are correct: it only proposes me real Classes (and not the part of the namespace I am looking for).

Proposition of a new behavior

Considering the following PHP code example:

<?php

namespace App\Foo\Bar;

class Baz
{
    public __construct(
        Finder\User $userFinder,
        User $otherUserArgument
    ) {
        $this->userFinder = $userFinder;
        $this->otherUserArgument = $otherUserArgument;
    }
}
Einenlum commented 5 years ago

The vim plugin change would be the simplest one:

function! phpactor#UseAdd()
    let iskeyword_backup=&iskeyword " Backup the normal iskeyword option
    set iskeyword+=\\ " Vim will take in account the \ in the word
    let word = expand("<cword>") " This can now take `Lorem` typehint or `Lorem\Ipsum\Foo\Bar`
    let &iskeyword=iskeyword_backup " We set back iskeyword to its original value not to mess up vim's behavior
    call phpactor#rpc("import_class", {"name": word, "offset": phpactor#_offset(), "source": phpactor#_source(), "path": expand('%:p')})
endfunction

Most of the change would have to be done in Phpactor\Extension\SourceCodeFilesystemExtra\SourceCodeFilestem\Application\ClassSearch then.

dantleech commented 5 years ago

This is the code which gets the suggestions:

https://github.com/phpactor/phpactor/blob/develop/lib/Extension/CodeTransformExtra/Rpc/ImportClassHandler.php#L149

It shouldn't really be there, and I want to extract it and subsequently support importing functions/constants. Still not sure about the best option for this, but for now it could even be extracted into a class within the extension (and maybe "moved" somewhere later)

dantleech commented 5 years ago

The vim plugin change would be the simplest one:

I would even say that we should not pass the word at all, and just pass the offset and the source code.

dantleech commented 5 years ago

the vim plugin only uses the word under the cursor to use classes.

the VIM plugin sends the offset to Phpactor, Phpactor falls back to the word under the cursor Oops, no it doesn't :flushed:


This should be a relatively easy fix -- I'll investigate

dantleech commented 5 years ago

This PR fixes the behavior so that:

dantleech commented 3 months ago

FWIW the LSP (and I guess the VIM plugin) will now autocomplete relative path segments.