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 support for Vim's tagstack to ALEGoToDefinition #2448

Merged
merged 2 commits into from
Apr 29, 2019

Conversation

reedriley
Copy link
Contributor

Fixes #1236.

I added tests - but to be fair, this feature won't work on versions of Vim earlier than 8.1.0519 and the test automation script doesn't test on anything that recent.

The feature is designed to degrade gracefully; and I'm open to suggestions on how to best clarify the version requirement in the docs.

@reedriley
Copy link
Contributor Author

I modified the Dockerfile and ./run-tests script; and there are issues with my proposed unit test. I'll put up a corrected version shortly.

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.

We should add tests for this. I'll have to update the Docker image so the 8.1 version we build is new enough to contain this feature.

let l:should_update_tagstack = exists('*gettagstack') && exists('*settagstack') && g:ale_update_tagstack

if l:should_update_tagstack
let l:from = [bufnr('%'), line('.'), col('.'), 0]
Copy link
Member

Choose a reason for hiding this comment

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

You can use the filename, line, and column we already have to determine this.

Copy link
Contributor Author

@reedriley reedriley Apr 24, 2019

Choose a reason for hiding this comment

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

Assuming you're referring to l:filename, l:line and l:column defined in ale#definition#HandleTSServerResponse and ale#definition#HandleLSPResponse - these are the targets for the new location. The values I'm pushing on to the stack are based on the old location.

I'll change the variable names to be a bit more clear. 😄

if l:should_update_tagstack
let l:from = [bufnr('%'), line('.'), col('.'), 0]
let l:tagname = expand('<cword>')
let l:winid = win_getid()
Copy link
Member

Choose a reason for hiding this comment

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

Get the window ID from the buffer number, instead of the current window.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This sounds like it's probably really relevant and important - but I'm afraid I'm not an expert on the Vim functions involving windows and buffers. Any chance you can provide some pointers to the relevant docs?


if l:should_update_tagstack
let l:from = [bufnr('%'), line('.'), col('.'), 0]
let l:tagname = expand('<cword>')

This comment was marked as resolved.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same as above - the values I'm pushing on to the stack are based on the old location. So I don't think I need to wait for the buffer to open; assuming I've understood this comment correctly?

@reedriley
Copy link
Contributor Author

We should add tests for this. I'll have to update the Docker image so the 8.1 version we build is new enough to contain this feature.

Agreed. The test I added is fairly minimal; but passes when I build a custom Docker image and update the run-tests script to use a local image. I'm happy to include the Dockerfile change as well, if you'd like.

The `settagstack` and `gettagstack` functions don't exist prior to Vim
8.1.0519.  And the function definition was unclear whether it intended
  to grab the *old* or the *new* file/line/col.
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'll merge this, and people can try it out.

@w0rp w0rp merged commit 2f3bce5 into dense-analysis:master Apr 29, 2019
@rislah
Copy link

rislah commented May 19, 2019

@w0rp @reedriley doesn't seem to be working for me. :ALEGoToDefinition and when I C-t then it says tagstack is empty. So right now I use C-o

@reedriley
Copy link
Contributor Author

@w0rp @reedriley doesn't seem to be working for me. :ALEGoToDefinition and when I C-t then it says tagstack is empty. So right now I use C-o

What version of Vim are you running?

@wrachwal
Copy link

@w0rp @reedriley I have the same problem (tagstack is empty). I am using Vim 8.1. Fresh ALE, commit * 27146ad 2019-05-30

@reedriley
Copy link
Contributor Author

@wrachwal: To be clear, I actually meant to ask which version of Vim. The tagstack manipulation functions the diff uses were added in Vim 8.1.0519.

@wrachwal
Copy link

wrachwal commented Jun 3, 2019

Thanks, after upgrade using ppa:jonathonf/vim it works fine.

OLD VIM - Vi IMproved 8.1 (2018 May 18, compiled Nov 6 2018 09:45:53)
Included patches: 1-513

NEW VIM - Vi IMproved 8.1 (2018 May 18, compiled May 27 2019 13:46:23)
Included patches: 1-1408

@dmchoiboi
Copy link

Is there any plan to provide this functionality for NeoVim? I'm on the latest NeoVim version (v0.3.7) and I'm getting the same 'tagstack is empty' mentioned above

@rislah
Copy link

rislah commented Jun 3, 2019

Yep its not working on neovim.

@hourliert
Copy link

Same issue here. It is not working on neovim 0.3.8. Can we make it work for neovim by any chance? Thanks in advance!

@dmchoiboi
Copy link

@hourliert @rislah I just checked NeoVim's source, and it appears that their patch for vim-8.1.0519 hasn't made it out to a stable release yet. According to that pull request, the update is targeted to be out in the 0.4 release. I've tested out the nightly release which contains the fix and verified that this tagstack feature works correctly.

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.

Request: add to tag stack when using ALEGoToDefinition OR add a ALEGoBackFromDefinition
6 participants