Completion tests

[xendk]

This adds tests for classes in sub-directories, completion in composer projects, and fixes some small bugs.

[xendk]

Bah, flycheck/emacs-travis failed...

[stevenremot]

Hey, sorry for the delay ! Yes, I won't merge the PR until the travis build is working, but we can start from this branch to implement all the features you described in your tests. You did an awesome job on this one 👍

The reason there was no completion test in the composer project is that the composer part of ede-php-autoload is only about automatically creating autoload configuration from a composer file. You can do the same configuration by hand if you want, and you still get all the features of ede-php-autoload.

So doing completion tests on the composer project doubles the number of tests without adding any value in my opinion. What do you think?

[xendk]

Hmm, yes you're right. However, the composer version did manage to flush out one more bug because the composer.json used a feature not tested in the without-composer project. I'll move it to the other file, but maybe the projects are overloaded with too many different scenarios to test?

It's also related to how one defines ones features. There's a case for having a psr-0.feature that tests both class detection and completions. But then you're not testing that it might co-exist with psr-4.

Any idea what's up with flycheck/emacs-travis? Seems like it broke overnight, and it was too late in the evening for me to dig into.

[stevenremot]

I had a look at the travis, It failed to download Emacs I think. I will relaunch the build.

[xendk]

The flycheck/emacs-travis was a bug that's now fixed. The good news is that builds should be generally faster.

I've implemented the completion structure. It's not particularly pretty, but it works, and the tests confirm this.

[stevenremot]

Thanks a lot for your hard work on it, to me we are nearly here ! Additionnaly to the tiny tweaks I asked, The new completion system requires changes in the semanticdb backend in order to work. The changes are pretty simple, but tell me if you need help on it 🙂

[stevenremot]

What about writing in this docstring the different values of type ? That way we have a reference of all these values

[xendk]

Done, but I have grown doubts (see follow up comment).

[stevenremot]

I think this function can be public and considered to be a helper to create autoloaders. What do you think ?

[xendk]

Sure, I've removed a dash. Should it still live in core.el?

[stevenremot]

What about something like (file-regular-p file-name)? That's my opinion but the more I avoid tests with regular expression the better I feel.

[xendk]

Well, file-name is just the plain name, and ede-php-autoload--gather-relative-subfiles might have found the file in any number of directories.

Changed it to string-suffix-p, how do you like that?

(also, see follow up comment)

[stevenremot]

Ha, that's convenient!

[stevenremot]

Great job on this one, the tests are really readable !

[xendk]

I've pushed changes. But I have some thoughts...

Regarding :namespace, :directory and :file, I realize it's the wrong vocabulary. We're not completing files or directories, we're completing classes and interfaces in namespaces.

For starters I don't think there's any reason to distinguish between the namespaces and directories. They're pretty much the same thing, and for my completion, for instance, it would suffice. As it stands, namespaces might return multi-component items, but that's just how it is.

Finding the proper vocabulary is an issue though. :namespace works for namespaces and directories. But what to call files is an issue, as they could be both classes, interfaces and traits, and calling them :class feels a bit wrong.

Having cut down the possibilities to two opens another possibility: Just having a 'finalboolean that'll benil` for namespaces and directories, and t for files.

What do you think?

The other thing: More than once while implementing think I wanted to move the classification between directories and file up into ede-php-autoload--gather-relative-subfiles to avoid jumping through hoops trying to figure out the type of each completion.

On the other hand, it's pretty generic and just collect names from a list of directories.

It could create the completions and remove much of the logic from the class loaders, but then it would need to know the namespace name to (to create the full name). Though then it should be named ede-php-autoload--gather-relative-completions.

Sudden epiphany

Haven't we gone all the wrong way about this? Shouldn't completion just return namespaces and directories with a backslash appended? When mucking about with my completer, I was annoyed by having to append the backslash manually before it could complete the next element. Appending the backslash would make it behave as intended, and seems to me to be the usual way to do things. How would the semantic completion like this?

We'd could still return a completion structure to return both the plain completion and the full name, or we could make semanticdb-find-tags-for-completion-method simply strip the prefix from the completions. How does that work anyway, seems to me that it has an issue with figuring out what is actually completes, unless it assumes it was whatever was after the last backslash? (And I've also realized my fix to it was stupid :-) )

[xendk]

Hah, and string-suffix-p doesn't work in 24.3.

[xendk]

I tried implementing the other completion, and it was going rather well, until I discovered that the PSR0 loader doesn't quite live up to the spec: http://www.php-fig.org/psr/psr-0/

It's only in class names that the underscores can be used as separator, so the original test that checks if Psr0Ns_T to Psr0Ns_TheClass is wrong. I then carried that misunderstanding on in the subdir handling.

Trying to fix that up gave me all kind of troubles, so before carrying on, I'd like to ask how important underscore completion is? Or could we get away with just always completing with backslashes? I mean, following the specs, Psr0Ns\TheS should offer both Psr0Ns\TheSubdir_ and Psr0Ns\TheSubdir\ as possible completions, and that's, IMHO, just ugly. And if you're using PSR4, backslashes should work just fine.

[stevenremot]

Regarding :namespace, :directory and :file, I realize it's the wrong vocabulary. We're not completing files or directories, we're completing classes and interfaces in namespaces.

For starters I don't think there's any reason to distinguish between the namespaces and directories. They're pretty much the same thing, and for my completion, for instance, it would suffice. As it stands, namespaces might return multi-component items, but that's just how it is.

Finding the proper vocabulary is an issue though. :namespace works for namespaces and directories. But what to call files is an issue, as they could be both classes, interfaces and traits, and calling them :class feels a bit wrong.

Having cut down the possibilities to two opens another possibility: Just having a 'finalboolean that'll benil` for namespaces and directories, and t for files.

What do you think?

Yes, you're right, I thought something was odd but I couldn't find what!

The other thing: More than once while implementing think I wanted to move the classification between directories and file up into ede-php-autoload--gather-relative-subfiles to avoid jumping through hoops trying to figure out the type of each completion.

On the other hand, it's pretty generic and just collect names from a list of directories.

It could create the completions and remove much of the logic from the class loaders, but then it would need to know the namespace name to (to create the full name). Though then it should be named ede-php-autoload--gather-relative-completions.

This, or you create another function that can create the completions from the result of ede-php-autoload--gather-relative-subfiles. This way we have two "simple" functions instead of one big function.

What do you think?

Haven't we gone all the wrong way about this? Shouldn't completion just return namespaces and directories with a backslash appended? When mucking about with my completer, I was annoyed by having to append the backslash manually before it could complete the next element. Appending the backslash would make it behave as intended, and seems to me to be the usual way to do things. How would the semantic completion like this?

We'd could still return a completion structure to return both the plain completion and the full name, or we could make semanticdb-find-tags-for-completion-method simply strip the prefix from the completions. How does that work anyway, seems to me that it has an issue with figuring out what is actually completes, unless it assumes it was whatever was after the last backslash? (And I've also realized my fix to it was stupid :-) )

I think your proposition of having a final flag is better, because relying on the presence of a backslash in the name is a bit too implicit for me. However I agree with you, appending a backslash to full namespaces may be a good idea.

Hah, and string-suffix-p doesn't work in 24.3.

Duh... What was the problem with file-regular-p then ? Or file-name-extension ?

I tried implementing the other completion, and it was going rather well, until I discovered that the PSR0 loader doesn't quite live up to the spec: http://www.php-fig.org/psr/psr-0/

It's only in class names that the underscores can be used as separator, so the original test that checks if Psr0Ns_T to Psr0Ns_TheClass is wrong. I then carried that misunderstanding on in the subdir handling.

Trying to fix that up gave me all kind of troubles, so before carrying on, I'd like to ask how important underscore completion is? Or could we get away with just always completing with backslashes? I mean, following the specs, Psr0Ns\TheS should offer both Psr0Ns\TheSubdir_ and Psr0Ns\TheSubdir\ as possible completions, and that's, IMHO, just ugly. And if you're using PSR4, backslashes should work just fine.

Ah, that's true. We can open another issue on that, but I think the impact is pretty limited because:

• PSR-0 is deprecated
• Most of the time, you use either namespaces or big class names, so there is not that much risk of mixing stuffs. At least that's my experience

[xendk]

This, or you create another function that can create the completions from the result of ede-php-autoload--gather-relative-subfiles. This way we have two "simple" functions instead of one big function.

What do you think?

Well, if ede-php-autoload--gather-relative-completions calls ede-php-autoload--gather-relative-subfiles it has the same problem of not knowing in which directory ede-php-autoload--gather-relative-subfiles found the file, so it can't check the type of the file. It can check the extension, but I'm not really fond of that as non-php files will show up as "directories" (which will probably cause an error if someone tries to complete on it). I know it's an edge-case, but I do like the simplicity of just checking for directories.

I think your proposition of having a final flag is better, because relying on the presence of a backslash in the name is a bit too implicit for me. However I agree with you, appending a backslash to full namespaces may be a good idea.

Too implicit? It's worked rather nicely for filesystems since forever. :-)

But I now realize there's two flavors of completion involved here. The full namespaced class completion I'm looking at, in order to identify a specific class in the global namespace, and the inline completion that semantic should eventually give us that takes the imported namespaces at the current buffer location into account.

I believe both kinds should be able to be transmogrified to the other, but I'll grant you that it's probably a tad more effective to create a new global completion function that makes a list of full names from per-component completions, rather than explode full names into inline completions.

And I still think the trailing backslash notation for sub-namespaces/directories is the simplest and most effective way to convey the structure.

So, a global completion function, would be a method on the ede-php-autoload-class-loader base class (which basically just calls ede-php-autoload-complete-type-name on itself and appends the prfix to all completions), or would you rather have a regular function (don't like the asymmetry of that).

As either completion function only need to return either the full or the component name, we can get back at simply returning a list of completions if we use the trailing backslash notation, which means that there's less munging needed in the glue-code to completion systems (which I'd argue would just use the :final property to determine whether to add a backslash themselves).

If you ask me, a thing of beauty. :-)

Duh... What was the problem with file-regular-p then ? Or file-name-extension ?

The problem was that the code that creates the completion has no idea in which directory ede-php-autoload--gather-relative-subfiles found the given completion in. file-name-extension could work though.

Ah, that's true. We can open another issue on that,

#28 it is...

[stevenremot]

Well, if ede-php-autoload--gather-relative-completions calls ede-php-autoload--gather-relative-subfiles it has the same problem of not knowing in which directory ede-php-autoload--gather-relative-subfiles found the file, so it can't check the type of the file. It can check the extension, but I'm not really fond of that as non-php files will show up as "directories" (which will probably cause an error if someone tries to complete on it). I know it's an edge-case, but I do like the simplicity of just checking for directories.

Why not passing the root directory as a parameter then? I don't have the code in mind, so I don't know whether or not it's possible.

Too implicit? It's worked rather nicely for filesystems since forever. :-)

I would remove the "nicely" and just say that it works :-D . I may be wrong, but I think most programs doesn't rely on the final backslash to know whether or not the path is a directory.

If you want another argument in favor of the flag, each library which uses the completion system will have to check for the presence of this trailing backslash to know whether or not it's a namespace. Personaly, I consider tests on strings as something risky because there is absolutely no guaranty of anything in a string. We have the possibility to easily give this information without relying on string operations, and ytou already implemented 90% of the solution, so I'm really in favor of keeping it.

But I now realize there's two flavors of completion involved here. The full namespaced class completion I'm looking at, in order to identify a specific class in the global namespace, and the inline completion that semantic should eventually give us that takes the imported namespaces at the current buffer location into account.

I believe both kinds should be able to be transmogrified to the other, but I'll grant you that it's probably a tad more effective to create a new global completion function that makes a list of full names from per-component completions, rather than explode full names into inline completions.

And I still think the trailing backslash notation for sub-namespaces/directories is the simplest and most effective way to convey the structure.

So, a global completion function, would be a method on the ede-php-autoload-class-loader base class (which basically just calls ede-php-autoload-complete-type-name on itself and appends the prfix to all completions), or would you rather have a regular function (don't like the asymmetry of that).

As either completion function only need to return either the full or the component name, we can get back at simply returning a list of completions if we use the trailing backslash notation, which means that there's less munging needed in the glue-code to completion systems (which I'd argue would just use the :final property to determine whether to add a backslash themselves).

Mmmh, I will contradict myself here, but this looks like a simpler solution for your problem, even if there is no flag...

So I'm OK to try this! And what about having a function ede-php-autoload-namespace-p that takes a "completion result" in parameter and returns whether or not it's a namespace? It will look at the backslash, so still a string operation, but at least it's isolated in one function we can improve over time if needed.

[xendk]

Why not passing the root directory as a parameter then? I don't have the code in mind, so I don't know whether or not it's possible.

Because ede-php-autoload--gather-relative-subfiles takes a list of directories to search (you know, multi dir name spaces). :-)

So I'm OK to try this!

I'll get right on it!

And what about having a function ede-php-autoload-namespace-p that takes a "completion result" in parameter and returns whether or not it's a namespace? It will look at the backslash, so still a string operation, but at least it's isolated in one function we can improve over time if needed.

Sure, though I'm not sure when one would have a need for that? And isn't this one of those things that would make sense as a macro?

[xendk]

There, completely new version (hope that Travis likes it).

I named the method ede-php-autoload-complete as it's the "generic" completion method (the one that works as is expected by most completion methods), and kept the ede-php-autoload-complete-type-name as is.

There's an ugly hack in ede-php-autoload-complete for dealiing with ede-php-autoload-complete-type-name returning multiple component namespaces at top level, which I'm not particularly proud of. But I'd rather follow up with a PR to make ede-php-autoload-complete-type-name complete top-level namespaces component-wise.

And the feature file is getting too big. I'd suggest splitting it up into features per ps0, psr4 and classmap (and letting type completion and completion be different scenarios) . Splitting up into completion and type completion is another possibility, but rubs me the wrong way. What do you say, how do you see the "features"?

[xendk]

Oh, and we lost the need for columns in the feature specs, but I kept the vertical format. I think it reads better and it lends itself towards bulk editing with rectangles and multiple-cursors.

[stevenremot]

Hey, sorry for the delay ! I had a look at your changes, it's all good to me. Do you have something else to do on this PR or can I merge it ?

[xendk]

I can't think of anything that couldn't be new PR, so you can merge.

[stevenremot]

Done. THanks for the hard work !