Nothing Special   »   [go: up one dir, main page]

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

Add ability to manually trigger completion menu #2177

Merged
merged 5 commits into from
Jan 7, 2019

Conversation

jeremija
Copy link
Contributor
@jeremija jeremija commented Jan 3, 2019

Following the discussion in #1830, a part of ale#completion#GetCompletions was extracted into a new function: ale#completion#AlwaysGetCompletions. A new :ALEComplete command was added, along with a plug mapping: <Plug>(ale_complete). Auto-completion can be triggered manually whether or not g:ale_completion_enabled is set.

Sample bindings:

inoremap <silent> <C-Space> <C-\><C-O>:GetCompletions<CR>
" or
imap <C-Space> <Plug>(ale_complete) 

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.

Looks pretty good to me. 👍 Could you update the tests to include a test for the new command? Could you update the documentation for completion so it mentions the new command and mapping? You can link to the documentation for the command from the ale-completion section.

doc/ale.txt Outdated Show resolved Hide resolved
@jeremija
Copy link
Contributor Author
jeremija commented Jan 3, 2019

Hi @w0rp, I updated the docs and added the test. In the test I noticed that I cannot override the ale#completion#AlwaysGetCompletions() function without it being called first, because it is an autoloaded function. Do you know of a better way to do this, so that I can remove the initial call in the test?

Also, an unrelated appveyor test failed (it is also failing on the master branch):

(5/5) [EXECUTE] (X) ['/foo/bar', '/c', 'foobar'] should be equal to ['/foo/bar', '-c', 'foobar']

@w0rp
Copy link
Member
w0rp commented Jan 5, 2019

If you rebase your code, that test on Windows will pass.

You can replace the function in Vader tests. Look through existing tests for completion. You can define the function in Before or in an Execute block, and then reload the original function again with runtime autoload/ale/completion.vim in After.

@jeremija
Copy link
Contributor Author
jeremija commented Jan 5, 2019

I guess I'm confused why I get this error in Vim 8.1:

Vim(function):E746: Function name does not match script file name: ale#completion#AlwaysGetCompletions

for this simple test case:

Before:
  let g:get_completions_called = 0
  function! ale#completion#AlwaysGetCompletions() abort
    let g:get_completions_called = 1
  endfunction

Execute(ale#completion#AlwaysGetCompletions should be called when ALEComplete is executed):
  AssertEqual 0, g:get_completions_called
  ALEComplete
  AssertEqual 1, g:get_completions_called

But the following works:

Before:
  function! MockAlwaysGetCompletions() abort
    let g:get_completions_called = 0

    function! ale#completion#AlwaysGetCompletions() abort
      let g:get_completions_called = 1
    endfunction
  endfunction

  call MockAlwaysGetCompletions()

Execute(ale#completion#AlwaysGetCompletions should be called when ALEComplete is executed):
  AssertEqual 0, g:get_completions_called
  ALEComplete
  AssertEqual 1, g:get_completions_called

@jeremija jeremija force-pushed the jeremija/manual-autocomplete branch from dbaec53 to d6ef5cb Compare January 5, 2019 19:40
@w0rp
Copy link
Member
w0rp commented Jan 6, 2019

You'll need to add a runtime line before you define the function, probably, or try moving it to the Execute block. Have a look at existing test files. They do similar things and the tests pass.

@jeremija
Copy link
Contributor Author
jeremija commented Jan 6, 2019

Thanks, I think I've already tried that and it didn't work. Did you get a chance to take a look at the latest commit? I did something similar to what I wrote above and the test passes.

@w0rp w0rp merged commit 0fcd5e7 into dense-analysis:master Jan 7, 2019
@w0rp
Copy link
Member
w0rp commented Jan 7, 2019

Cheers! 🍻

@jeremija jeremija deleted the jeremija/manual-autocomplete branch January 7, 2019 22:21
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.

2 participants