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

Manually trigger autocomplete even when prefix is "" #2189

Merged

Conversation

jeremija
Copy link
Contributor

@jeremija jeremija commented Jan 8, 2019

This is also related to #1830. Adds a fix for the following scenario:

const x: T = {
  // <- Cursor rests here
}

Previously, the manual completion would not be triggered when the prefix was empty.

@jeremija jeremija force-pushed the jeremija/manual-autocomplete-wo-prefix branch from 07f7ca4 to 9bcf8a2 Compare January 8, 2019 11:57
Copy link
Member

@w0rp w0rp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought this would be an issue. It's good that you noticed it too and fixed it.

endfunction

" This function can be used to manually trigger autocomplete, even when
" g:ale_completion_enabled is set to false
function! ale#completion#AlwaysGetCompletions() abort
function! ale#completion#AlwaysGetCompletions(...) abort
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function isn't part of ALE's public API, so just add a new argument here and update the command. This is why I asked for the command to be part of the public API, but not the function. 😉

@@ -106,7 +106,7 @@ function! ale#completion#Filter(buffer, filetype, suggestions, prefix) abort
" foo.
" ^
" We need to include all of the given suggestions.
if index(l:triggers, a:prefix) >= 0
if index(l:triggers, a:prefix) >= 0 || empty(a:prefix)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of writing this here, move if empty(a:prefix) to the top of the function, before l:triggers is loaded, and return a:suggestions right away. That will be more efficient when there's no prefix.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In GetTriggerCharacter above, you should probably add if empty(a:prefix) ... return '' to the top of the function, to avoid processing we don't need again.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I said return a:suggestions right away. What I should say is set the variable right away without the other processing above. The suggestions still need to be filtered below, as some LSP configurations have filters in Vim for removing noise.

Copy link
Member

@w0rp w0rp Jan 8, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a test which ensures that filtering on suggestions for Python still works when there's no prefix.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Roger! When you say:

The suggestions still need to be filtered below, as some LSP configurations have filters in Vim for removing noise.

are you referring to the block starting with if !empty(l:excluded_words)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a test which ensures that filtering on suggestions for Python still works when there's no prefix.

Do you think the test I added to test_completion_filtering.vader is enough?

  AssertEqual
  \ ['Deutsch'],
  \ ale#completion#Filter(bufnr(''), '', ['Deutsch'], '')

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we the function shouldn't return early, I realised, and should continue down to the part for processing filtering with functions. Look through the tests and make sure that we have a test for the Python language server filtering function which still filters the results when the prefix is empty.

Copy link
Member

@w0rp w0rp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, this will work. 👍 Nevermind what I said about Python filtering, that's done in another function.

@w0rp w0rp merged commit 84475ff into dense-analysis:master Jan 8, 2019
@w0rp
Copy link
Member

w0rp commented Jan 8, 2019

Cheers! 🍻

@jeremija
Copy link
Contributor Author

jeremija commented Jan 8, 2019

Thanks!

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

Successfully merging this pull request may close these issues.

None yet

2 participants