jcollard / elm-mode

Elm mode for emacs
GNU General Public License v3.0
376 stars 67 forks source link

Improved auto completion #122

Closed mewa closed 6 years ago

mewa commented 7 years ago

As mentioned in #121, this PR contains an approach where modules' contents are cached in a global cache and aliased imports inside separate (per-file) caches.

The changes expose a common interface for querying for completion as well as a company backend which uses them.

This leaves some small changes to be made like updating the ac-based completion to use this new interface. Also completions for modules exposing (..) currently completes a fully qualified name.

I'll also need to document things a bit before they gets merged.

purcell commented 7 years ago

Great! I left a few little comments, and I'm looking forward to seeing where this goes!

purcell commented 7 years ago

I just reverted a commit in d4af45e, which will have caused a minor conflict for you - apologies. That commit had been causing #123, and contributed to #119.

mewa commented 7 years ago

I switched to your implementation for the imports reader and removed that buffer->string utility function as it wasn't needed anymore. I'll probably update the docs tomorrow.

purcell commented 7 years ago

There's probably also a little bonus fix possible here for the eldoc support. Right now that shows the doc hint for the first completion for the symbol at point, but that's not the correct behaviour, because it should only show the hint if the symbol is already complete. In the current code it's hard to determine that, but the new improved completion should make it much easier.

purcell commented 7 years ago

I tested this out locally in its current form and it works pretty nicely! :tada:

At first I only had company's capf backend enabled: elm-oracle-setup-completion is generally enough to make this work, but not any more, so I think elm-oracle-completion-at-point-function will need some fixing. (Relatedly, I know the ac backend was also on your radar.)

All in all, this is great - thanks for digging in and helping. :-)

purcell commented 7 years ago

Hey @mewa, in https://github.com/jcollard/elm-mode/commits/pr/122 I pushed https://github.com/jcollard/elm-mode/commit/8cc59c9af136ca5f6f960b21e4adc32a03b489c8, which goes most of the way to fixing the non-company completion backends to work with your code. I haven't done anything re. the other feedback I left for you. Maybe you'd like to cherry-pick the above commit into your branch to save yourself some work.

mewa commented 7 years ago

Ah, thanks, that's great news! Sorry I haven't been as responsive as I'd like to but I had a bit of work on my plate that just had to be done. I'll mege your changes in a moment.

purcell commented 6 years ago

Hey @mewa! Thanks for all your hard work on these changes. I've just merged this PR, having made a couple of further changes. There's some stuff that still isn't working perfectly, but it's such an improvement over the previous state of affairs that we should just move forward with this.

I tested the old completion system manually against this new one.

Given these imports:

import RouteUrl.Builder as BuildUrl
import Date
import RemoteData exposing (RemoteData(..))
import List.Extra as ListX
import Maybe.Extra as MaybeX exposing (isNothing)

here are the cases I ran through with their results:

Input Completion expected Correct in old code Correct in new code
Maybe.Extra None because imported aliased N (completes Maybe.Extra.*) Y
MaybeX MaybeX.andMap etc. N (empty) Y
isSuccess RemoteData.isSuccess N (empty) Y
Success Success N (empty) N (get: RemoteData.Success)
Mon Date.Mon N (empty) Y
Dat Date N (empty) N
Date.Dat Date.Date N (empty) N
isNothing isNothing N (empty) Y (but not first result)
Date Date.Mon etc. N (empty) Y
Date. Date.Mon etc. N (empty) Y
map List.map etc. N (empty) N (List.map is returned unprefixed)
build BuildUrl.build etc. N (emtpy) N (returned unprefixed)
purcell commented 6 years ago

Ugh, this seems broken when jumping between files that have different import lists. :-/ Will investigate and fix.

purcell commented 6 years ago

Okay, I fixed that now by simplifying the caching code. We now store a buffer-local pair of the imports list and the corresponding elm-oracle results for the empty search, and if the imports list changes, the elm-oracle results are refreshed.

I also fixed the annoying thing where eldoc hints would show up for an item at point even if it wasn't an exact match to something that elm-oracle knew about.

purcell commented 6 years ago

I've done a bunch more work on this, which is all in master -- please check it out and let me know if you encounter any issues.

mewa commented 6 years ago

Thanks! I'll have a look in a couple days probably

mewa commented 6 years ago

I finally had some time to evaluate it while working on an elm project and so far it seems good :smile:

purcell commented 6 years ago

Yep, indeed seems pretty nice. Good team work - thanks for stepping up!