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

Update ncm to ncm2 #15

Merged
merged 4 commits into from
Jul 23, 2019
Merged

Conversation

FatBoyXPC
Copy link
Contributor

@FatBoyXPC FatBoyXPC commented Jul 20, 2019

Note: this will break for anybody that currently uses ncm. I wasn't sure the right way to handle this - add a whole new group of stuff for ncm2, or just update it. I figured I would update it first since that was the easier path.

I tried to run the tests but bundle install failed, stating: Could not locate Gemfile

In order to get the routes completion working for ncm2, I had to make
the capture() function available from anywhere, and I needed to call it
with 2 arguments intead of 1.

In order to get the routes completion working for ncm2, I had to make
the capture() function available from anywhere, and I needed to call it
with 2 arguments intead of 1.
Copy link
Owner

@noahfrederick noahfrederick left a comment

Choose a reason for hiding this comment

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

Thanks for taking this on!

I wasn't sure the right way to handle this - add a whole new group of stuff for ncm2, or just update it.

Let's preserve the original ncm support, since the cost is low. But you can stick new autocommands in the same group (laravel_completion).

plugin/laravel.vim Outdated Show resolved Hide resolved
plugin/laravel.vim Outdated Show resolved Hide resolved
@noahfrederick
Copy link
Owner

I tried to run the tests but bundle install failed, stating: Could not locate Gemfile

Not sure where this instruction came from, but there are none, unfortunately. :/

@FatBoyXPC
Copy link
Contributor Author

That instruction came from CONTRIBUTING.md!

@FatBoyXPC
Copy link
Contributor Author

Made the requesting changes, let me know if there's anything else!

Copy link
Owner

@noahfrederick noahfrederick left a comment

Choose a reason for hiding this comment

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

Looks good after these few formatting nitpicks.

I actually can't get it to work, but my autocompletion setup is perpetually broken in one way or another. As long as you've manually confirmed this works, I'm happy to merge it.

\ 'on_complete': 'laravel#completion#ncm2_routes',
\ })
autocmd User Ncm2Plugin call ncm2#register_source({
\ 'name' : 'Laravel View',
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
\ 'name' : 'Laravel View',
\ 'name': 'Laravel View',

" Set up ncm2 sources
" :help ncm2#register_source-example
autocmd User Ncm2Plugin call ncm2#register_source({
\ 'name' : 'Laravel Route',
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
\ 'name' : 'Laravel Route',
\ 'name': 'Laravel Route',

\ 'name' : 'Laravel Route',
\ 'priority': 9,
\ 'subscope_enable': 1,
\ 'scope': ['php','blade'],
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
\ 'scope': ['php','blade'],
\ 'scope': ['php', 'blade'],

\ 'name' : 'Laravel View',
\ 'priority': 9,
\ 'subscope_enable': 1,
\ 'scope': ['php','blade'],
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
\ 'scope': ['php','blade'],
\ 'scope': ['php', 'blade'],

Honestly I shouldn't have missed these to begin with :O
@FatBoyXPC
Copy link
Contributor Author

It does work for me locally, but I wouldn't mind somebody else trying. Do you know anybody else using ncm2? How is your autocomplete currently broken? Maybe we could fix it and test my changes?

@noahfrederick
Copy link
Owner

Okay, I was able to create a working minimal configuration. Strangely, ncm2 is including view names (as well as the route names) in the route context and vice versa.

Screen Shot 2019-07-20 at 9 39 28 PM

@FatBoyXPC
Copy link
Contributor Author

That's strange, that doesn't happen for me. Can you share your minimal configuration so I can also try it?

@noahfrederick
Copy link
Owner

noahfrederick commented Jul 21, 2019

Replace paths to local copies of plug-ins as needed:

call plug#begin()
call plug#('roxma/nvim-yarp')
call plug#('ncm2/ncm2')

call plug#('~/src/vim-composer')
call plug#('~/src/vim-laravel')
call plug#end()

augroup init_ncm2
  autocmd!
  autocmd BufEnter * call ncm2#enable_for_buffer()
augroup END

set completeopt=menuone         " Display menu when completing, even for only one candidate
set completeopt+=noselect       " Do not automatically select first item
set completeopt+=noinsert       " Do not automatically insert first item

Starting Neovim 0.3.8 in a Laravel project with:

nvim -u path/to/minimal.vim app/Http/Controllers/Controller.php

@noahfrederick
Copy link
Owner

I've been reading through the documentation. I think the complete_pattern key doesn't scope the completions in the way the original ncm did, but rather just provides extra contexts for automatically popping up the menu. I think we may need to check the context ourselves before returning results for a particular completion type, which is a little bit of a drag.

@FatBoyXPC
Copy link
Contributor Author

Hm, I do see this bug now, but only after I call route() a 2nd time. I'm attempting the minimal configuration right now, but I'm getting a lot of errors. Did you do a :PlugClean and :PlugInstall with that config before attempting this?

@noahfrederick
Copy link
Owner

Did you do a :PlugClean and :PlugInstall with that config before attempting this?

I did not, since it's a strict subset of my normal config. It shouldn't make a difference in my case.

but I'm getting a lot of errors.

Can you tell from what script?

@FatBoyXPC
Copy link
Contributor Author

The only difference is that I commented out the vim-composer line because I don't use it (and it's not necessary for what I use this plugin for).

Here's a gif of what's happening:

2019-07-20_22:33:00_fatboyxpc

@FatBoyXPC
Copy link
Contributor Author

Hey, I found this in the docs:

    complete_length                 ncm2-complete_length                        
                        The minimum length of the matching word for auto        
                        triggering popup menu.                                  
                        If it contains a negative value, completion will only   
                        be triggered by complete_pattern.

This part being important:

If it contains a negative value, completion will only be triggered by complete_pattern.

Care to pull my changes and see if this is fixed?

Copy link
Owner

@noahfrederick noahfrederick left a comment

Choose a reason for hiding this comment

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

Works as expected now!

@noahfrederick noahfrederick merged commit 2fdaf92 into noahfrederick:master Jul 23, 2019
noahfrederick added a commit that referenced this pull request Sep 12, 2020
* Fix :Artisan make:model in Laravel 8.
* Add support for artisan make:channel generator introduced in Laravel
  5.6.
* Dynamically complete Artisan flags.
* Fold stack traces in log buffers.
* Make gf go to intended config/translation file.
* Implement completion via ncm2 (#15).
* Always set up job/command nav commands.
* Various bug fixes.
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