Skip to content
This repository has been archived by the owner on Mar 6, 2022. It is now read-only.

[POC][Improvement] completion handle aliases #33

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

camilledejoye
Copy link
Contributor

@camilledejoye camilledejoye commented Nov 28, 2020

Hi,

That's an attempt to deal with aliases from the import table when completing classes (see phpactor/phpactor#1121).
I tried to not keep it as clean as possible but it still kind of a POC to see how you feel about the idea so don't hesitate: the more critics the better 😄

I decided to put it on the ChainTolerantCompletor because we need the parser to get the import table and I wanted it to work for all class completors without having to deal with them one by one.

The new DoctrineAnnotationCompletor is a bit special so I had to extract the logic into a trait.
As I said in the commit message I did it to because it feels a right use case but if you prefer an helper class with static methods it's also possible of course.

I've mainly used the automated test for now, just one or two manual test in a demo project to make sure it works.
I will update my work environment on Monday so that I can test it in a more realistic code base.

Todos:

Before all I did was trying to find the most accurate name to use
depending on the import table.
It actually not the job of the completor but the responsibility of the
user.

This commit will instead resolve a class suggestion to a list of
suggestions in order for the user to pick the one he wants.
I want to extract the logic in order to reuse it in
DoctrineAnnotationCompletor
Inheritance is not suited in this case, the only remaining options I can
think of are:
* Using a static method in an helper class
* Injecting a new dependency

I don't think injecting a dependency is a good solution here since I
can't think of a different implementation I might want to inject.
The helper class with a static method is a possibility but I think trait
are also meant for scenarios like this one.

So I kept the trait to open a new debate :)
@camilledejoye camilledejoye changed the title [Improvement] completion handle aliases [POC][Improvement] completion handle aliases Nov 28, 2020
@dantleech
Copy link
Contributor

Instead of suggesting on En<> => ORM\Entity would it be an idea to instead complete on ORM\\{Entity,Column,Foobar} ? This also makes annotations discoverable ?

@camilledejoye
Copy link
Contributor Author

Yes, that was the first item in phpactor/phpactor#1121
I just think it might be a bit harder right now because of how the name lookup works but I would definitely like to add this at some point.

@dantleech
Copy link
Contributor

dantleech commented Nov 29, 2020

Searching by FQN is trivial to add fortunately ☺️

image

See PR phpactor/indexer-extension#26

@dantleech
Copy link
Contributor

dantleech commented Nov 29, 2020

Given the above is now merged -- would it make sense to do namespace and import name completion instead of this?

  • FQN completion:
    • Can be clearly applied (e.g. only when the node is already a partially qualified name ORM\\<>)
    • Can help with discovery
    • Requiring some specification helps (e.g. if I search for Before<> with PHPBench + Behat + etc installed the list is quite extensive)
  • Import name completion: e.g. use Doctrine\Annotations; Anno<> => Annotations\\

Not against this - but if this can be accomplished in another way it might be redundant to have?

@camilledejoye
Copy link
Contributor Author

For me this is two complementary features.
The one in this PR allows to discover aliases that you don't even realize are used and it's more convenient.

With only the search by relative qualified name we still need to type the alias like ORM\.
With Symfonys validator it could be really painful to always have to retype the prefix when you could simply do NotBl<> and have it completed properly given the import table.

While the improvement you've done on the indexer will indeed grant us the possibility to discover available names, which is always useful in new/large code base.
It does not allow to discover existing aliases, often at work I want to import a class but someone already did but used an alias for some reason. When typing the "correct" class name I'm not aware of this alias: this feature will show me a different name letting me know about the alias.

I grant you the last one is really an edge case :)

Bottom line I think we can keep this one, with upgrade if necessary, and create another one to complete relative qualified name.

@dantleech
Copy link
Contributor

you could simply do NotBl<> and have it completed properly given the import table.

image

Maybe it should be prioritized?

@dantleech
Copy link
Contributor

But that can be a separate thing -- we should add:

$container->register(CompletorClass::class, function () {}, [ CompletionExtension::COMPLETOR => [ 'priority' => 100]])

in the completion extension

We want to have a dedicated completor for the resolved import
suggestions.

The idea is to register multiple time the same completor but with
different dependencies:
NameSearcherCompletor         => Find names with indexer and genrate the suggestions
TolerantNameSearcherCompletor => complete class names using a NameSearcherCompletor
DoctrineAnnotationCompletor   => complete annotations using a NameSearcherCompletor
ImportedNameSearcherCompletor => complete with import table from the suggestions of a NameSearcherCompletor

Completors:
TolerantNameSearcherCompletor(NameSearcherCompletor)
DoctrineAnnotationCompletor(NameSearcherCompletor)

TolerantNameSearcherCompletor(ImportedNameSearcherCompletor)
DoctrineAnnotationCompletor(ImportedNameSearcherCompletor)
@camilledejoye
Copy link
Contributor Author

camilledejoye commented Nov 29, 2020

I updated with the implementation we discussed earlier.
I will create a PR right away for the completion-worse-extension so that you can test it without figuring out what to register as a service.
Works for me, I will gave it a try at work as much as possible to have a better feeling about it next week-end ;)

PR for completion-worse-extension: phpactor/completion-worse-extension#4

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants