[vim/vim] runtime:vim: make K / &keywordprg smarter (PR #16680)
Enno
Let keywordprg in vim.vim to handle context-sensitive help calls. Detect the syntax group of the word under the cursor.
See #16677
You can view, comment on, or merge this pull request online at:
https://github.com/vim/vim/pull/16680
Commit Summary
- 45f9828 runtime:vim: make K / &keywordprg smarter
File Changes
(1 file)
- M runtime/ftplugin/vim.vim (19)
Patch Links:
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
ubaldot
ubaldot left a comment (vim/vim#16680)I really like it! But with all the due respect, wouldn't be better to provide new contributions in Vim9? :)
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Christian Brabandt
chrisbra left a comment (vim/vim#16680)yes and no. We need to be a bit careful, since those runtime files are also consumed by Neovim.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
ubaldot
ubaldot left a comment (vim/vim#16680)Hum, how do we distinguish how to send contribution then?
Is it "if you know both vim9 and legacy vim language then use the old language" rule that one should respect?
Out of curiosity: do Neovim devs apply the same rule by replacing "vim9" with "lua"?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Enno
Konfekt left a comment (vim/vim#16680)I really like it! But with all the due respect, wouldn't be better to provide new contributions in Vim9? :)
Since one can't mix well in a single file, how about a ftplugin/vim_9.vim ?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Andrew Radev
@AndrewRadev commented on this pull request.
> @@ -48,7 +48,22 @@ setlocal fo-=t fo+=croql
setlocal isk+=#
" Use :help to lookup the keyword under the cursor with K.
-setlocal keywordprg=:help
+" Distinguish between commands, options and functions.
+function! s:Help(args)
+ silent! let syn_name = synIDattr(synID(line('.'), col('.'), 1), 'name')
+
+ if syn_name =~# 'vimCommand'
+ execute 'help' a:args..':'
⬇️ Suggested change
- execute 'help' a:args..':' + execute 'help :'..a:args
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
ubaldot
ubaldot left a comment (vim/vim#16680)I really like it! But with all the due respect, wouldn't be better to provide new contributions in Vim9? :)
Since one can't mix well in a single file, how about a
ftplugin/vim_9.vim?
Is vim_9 a valid filetype? Or your proposal is to create an entirely new filetype?
I am just speculating now, but if the intention was to keep both languages perhaps Bram himself already included a vim_9 filetype?
On the contrary, I think the idea was to slowly phase out the old language, but it seems there are too many obstacles to make it to happen.
I understand to leave old stuff as-is (but, respectfully, I don't share this thought either, for me wherever contributors find some legacy vim part that they can replace on their way they should actually do it) but if we also have to accept new contributions in the old languages I don't see how Vim9 can ever flourish.
If we consider the number of reasons why things shall be done in legacy vim (old users, neovim support, consistency in the documentation, etc), then the number of valid use-cases for Vim9 becomes extremely small.
Was all the effort in developing Vim9 only for a very limited number of use-cases?
Is there any feasibility analysis done before starting the development of Vim9? If so, what was the outcome?
I may be missing something here, and I would gladly get some clarification.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Gary Johnson
> Let keywordprg in vim.vim to handle context-sensitive help calls. Detect the
> syntax group of the word under the cursor.
>
> See #16677
syntax highlighting is not enabled, you should revert to the normal
K operation.
I would suggest incorporating the parsing of the code surrounding
the selected word here rather than relying on syntax highlighting.
Regards,
Gary
vim-dev ML
vim-ml left a comment (vim/vim#16680)—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
D. Ben Knoble
@benknoble commented on this pull request.
> + silent! let syn_name = synIDattr(synID(line('.'), col('.'), 1), 'name')
+
+ if syn_name =~# 'vimCommand'
Since syntax may not be on, it seems prudent to perform the (probably cheaper) &syntax check first. Something like
if syntax on | execute 'help' a:args | return | endif
" Rest of what we have
—
Reply to this email directly, view it on GitHub.
You are receiving this because you commented.
Enno
Konfekt left a comment (vim/vim#16680)Yes, syntax highlighting is not a given, things can break. That's what silent! is for. Then synIDattr() returns an empty string and thus falls back to :help. Additional precautionary measures will be added to the maintainer's, @dkearns 's, liking
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you commented.
Enno
@Konfekt pushed 1 commit.
- 0c0e1d5 fix typo
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
Enno
Konfekt left a comment (vim/vim#16680)@ubaldot Is vim_9 a valid filetype? Or your proposal is to create an entirely new filetype?
It's somewhat of a trick, as any ftplugin/filetype_*.vim is loaded for filetype. In particular vim_9.vim for filetype vim.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you commented.
Enno
Konfekt left a comment (vim/vim#16680)On the contrary, I think the idea was to slowly phase out the old language, but it seems there are too many obstacles to make it to happen.
It will always stay around since the command line expects it. Due to Neovim using Lua, Legacy Vimscript is the common denominator, stalling phase-out.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you commented.
Christian Brabandt
@chrisbra commented on this pull request.
> @@ -48,7 +48,22 @@ setlocal fo-=t fo+=croql setlocal isk+=# " Use :help to lookup the keyword under the cursor with K. -setlocal keywordprg=:help +" Distinguish between commands, options and functions. +function! s:Help(args)
Please also add:
if exists("*" .. expand("<SID>") .. "Help")
finish
endif
> @@ -48,7 +48,22 @@ setlocal fo-=t fo+=croql
setlocal isk+=#
" Use :help to lookup the keyword under the cursor with K.
-setlocal keywordprg=:help
+" Distinguish between commands, options and functions.
+function! s:Help(args)
+ silent! let syn_name = synIDattr(synID(line('.'), col('.'), 1), 'name')
+
+ if syn_name =~# 'vimCommand'
+ execute 'help :'..a:args
+ elseif syn_name =~# 'vimOption'
+ execute 'help' "'"..a:args.."'"
⬇️ Suggested change
- execute 'help' "'"..a:args.."'" + execute "help '"..a:args.."'"
> @@ -48,7 +48,22 @@ setlocal fo-=t fo+=croql
setlocal isk+=#
" Use :help to lookup the keyword under the cursor with K.
-setlocal keywordprg=:help
+" Distinguish between commands, options and functions.
+function! s:Help(args)
+ silent! let syn_name = synIDattr(synID(line('.'), col('.'), 1), 'name')
+
+ if syn_name =~# 'vimCommand'
+ execute 'help :'..a:args
+ elseif syn_name =~# 'vimOption'
+ execute 'help' "'"..a:args.."'"
+ elseif syn_name =~# 'vimFunc'
+ execute 'help' a:args..'()'
inconsistent concat
> @@ -48,7 +48,22 @@ setlocal fo-=t fo+=croql
setlocal isk+=#
" Use :help to lookup the keyword under the cursor with K.
-setlocal keywordprg=:help
+" Distinguish between commands, options and functions.
+function! s:Help(args)
+ silent! let syn_name = synIDattr(synID(line('.'), col('.'), 1), 'name')
+
+ if syn_name =~# 'vimCommand'
+ execute 'help :'..a:args
+ elseif syn_name =~# 'vimOption'
+ execute 'help' "'"..a:args.."'"
+ elseif syn_name =~# 'vimFunc'
+ execute 'help' a:args..'()'
+ else
+ execute 'help' a:args
inconsistent concat
> + silent! let syn_name = synIDattr(synID(line('.'), col('.'), 1), 'name')
+
+ if syn_name =~# 'vimCommand'
I suppose you mean if g:syntax_on
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you commented.
D. Ben Knoble
benknoble left a comment (vim/vim#16680)@gary, @benknoble Yes, syntax highlighting is not a given, things can break. That's what
silent!is for. ThensynIDattr()returns an empty string and thus falls back to:help. Additional precautionary measures will be added to the maintainer's, @dkearns 's, liking
Right, I knew that. But it seemed wasteful (performance-wise) to go the current route with silent! and and if ladder instead of falling back to the normal path early when syntax isn't on (a cheap check). But maybe it doesn't matter. I like things to be snappy, and if syntax is off, I almost always do it for performance.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you commented.
D. Ben Knoble
@benknoble commented on this pull request.
> + silent! let syn_name = synIDattr(synID(line('.'), col('.'), 1), 'name')
+
+ if syn_name =~# 'vimCommand'
Right, pseudocode ;) should have indicated that better
—
Reply to this email directly, view it on GitHub.
You are receiving this because you commented.
Enno
@Konfekt commented on this pull request.
> @@ -48,7 +48,22 @@ setlocal fo-=t fo+=croql setlocal isk+=# " Use :help to lookup the keyword under the cursor with K. -setlocal keywordprg=:help +" Distinguish between commands, options and functions. +function! s:Help(args)
I am not sure. Doens't finish come a bit early here, since s:help will be defined on the first load of this file, and then the following settings no longer applied in other buffers?
—
Reply to this email directly, view it on GitHub.
You are receiving this because you commented.
Phạm Bình An
brianhuster left a comment (vim/vim#16680)Out of curiosity: do Neovim devs apply the same rule by replacing "vim9" with "lua"?
The Neovim rule is also similar, for runtime files shared by both Vim and Neovim, they should be written in legacy Vimscript. Any enhancement, bug fixes on those files have to be sent to Vim first (for example #16655).
The only exception is filetype detection, since Vim has rewritten it in Vim9script, Neovim has rewritten it in Lua.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you commented.
Maxim Kim
habamax left a comment (vim/vim#16680)The Neovim rule is also similar, for runtime files shared by both Vim and Neovim, they should be written in legacy Vimscript
I wouldn't say it is similar although might be wrong. I don't remember there was an "official statement" on this. I believe we are free to use vim9script in runtime files without looking into what is happening in neovim. It doesn't mean we have to use it though.
And in general, the idea of being limited by the downstream (or separate if you will) project is not very appealing. If that is the case, why not drop vim and just use neovim.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you commented.
Phạm Bình An
brianhuster left a comment (vim/vim#16680)I don't remember there was an "official statement" on this
Yes, there is none, but if you rewrite this PR in Lua and send it to Neovim, it will definitely be rejected.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you commented.
Maxim Kim
habamax left a comment (vim/vim#16680)Yes, there is none, but if you rewrite this PR in Lua and send it to Neovim, it will definitely be rejected quickly
And why it matters here in Vim? Don't get me wrong, I believe the old runtime files should not forcefully being rewritten in vim9script, but on the other hand if there is an indent ftplugin that would be faster or more manageable in vim9script and maintainer did change it -- it should not be rejected by vim.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you commented.
Phạm Bình An
brianhuster left a comment (vim/vim#16680)if there is an indent ftplugin that would be faster or more manageable in vim9script and maintainer did change it -- it should not be rejected by vim.
Of course it would not be rejected, in fact, some syntax files and filetype detection plugins are already written in Vim9script.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you commented.
Christian Brabandt
chrisbra left a comment (vim/vim#16680)yes where it makes sense (e.g. for performance reasons) re-writing in vim9 script is fine, especially if Neovim already implements their own solution in lua (like for the filetype detection logic).
In this particular case, I don't see the benefit for a simple change to rewrite in Vim9, but when we have an active maintainer, it's in the end his decision what he prefers. After all, that's why he is the maintainer.
In any case ping @dkearns for ACK'ing this change please :)
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you commented.
Phạm Bình An
brianhuster left a comment (vim/vim#16680)if there is an indent ftplugin that would be faster or more manageable in vim9script and maintainer did change it -- it should not be rejected by vim.
I don't think it would be rejected, in fact, some syntax files and filetype detection plugins are already written in Vim9script. Also dkearns is already writing some other files in Vim9script
Edit : I think there are a few cases Vim's bundled plugins can be written in Vim9script without affecting Neovim, including:
* [netrw.vim](https://github.com/saccarosium/netrw.vim) because Neovim has plan to freeze and replace it * Syntax files for `help` and `Lua` because Neovim now uses Treesitter highlighting for them by default. Also their FoldExpr functions (if there are) * `Editorconfig` plugin because Neovim one is written in Lua * `Vimball` and `logiPat` because it has been removed from Neovim
—
Reply to this email directly, view it on GitHub.
You are receiving this because you commented.
dkearns
dkearns left a comment (vim/vim#16680)It looks OK to me, thanks @Konfekt.
Are we aware that tpope has a more complete version of this included in his vim-scriptease plugin?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you commented.
Enno
Konfekt left a comment (vim/vim#16680)No, then take this one, likely more battle tested
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you commented.
Enno
Konfekt left a comment (vim/vim#16680)The only not self-contained part, scriptease#synnames() is in fact synIDattr(synID(line('.'), col('.'), 1), 'name')
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you commented.
Christian Brabandt
chrisbra left a comment (vim/vim#16680)@Konfekt are you making those changes?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you commented.
D. Ben Knoble
benknoble left a comment (vim/vim#16680)I haven't checked the terms of Vim's license in a while, but surely we need to attribute the source of the new code?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you commented.
Christian Brabandt
@chrisbra commented on this pull request.
> @@ -48,7 +49,49 @@ setlocal fo-=t fo+=croql
setlocal isk+=#
" Use :help to lookup the keyword under the cursor with K.
-setlocal keywordprg=:help
+" Distinguish between commands, options and functions.
+if !exists("*" .. expand("<SID>") .. "Help")
+ function s:Help(topic) abort
+ let topic = a:topic
+
+ if get(g:, 'syntax_on', 0)
+ silent! let syn = synIDattr(synID(line('.'), col('.'), 1), 'name')
why the silent!?
> - \ '\<if\>:\%(\%(^\||\)\s*\)\@<=\<el\%[seif]\>:\%(\%(^\||\)\s*\)\@<=\<en\%[dif]\>,' ..
- \ '{:},' ..
- \ '\<try\>:\%(\%(^\||\)\s*\)\@<=\<cat\%[ch]\>:\%(\%(^\||\)\s*\)\@<=\<fina\%[lly]\>:\%(\%(^\||\)\s*\)\@<=\<endt\%[ry]\>,' ..
- \ '\<aug\%[roup]\s\+\%(END\>\)\@!\S:\<aug\%[roup]\s\+END\>,' ..
- \ '\<class\>:\<endclass\>,' ..
- \ '\<interface\>:\<endinterface\>,' ..
- \ '\<enum\>:\<endenum\>'
+ \ '\<\%(fu\%[nction]\|def\)!\=\s\+\S\+\s*(:\%(\%(^\||\)\s*\)\@<=\<retu\%[rn]\>:\%(\%(^\||\)\s*\)\@<=\<\%(endf\%[unction]\|enddef\)\>,' ..
+ \ '\<\%(wh\%[ile]\|for\)\>:\%(\%(^\||\)\s*\)\@<=\<brea\%[k]\>:\%(\%(^\||\)\s*\)\@<=\<con\%[tinue]\>:\%(\%(^\||\)\s*\)\@<=\<end\%(w\%[hile]\|fo\%[r]\)\>,' ..
+ \ '\<if\>:\%(\%(^\||\)\s*\)\@<=\<el\%[seif]\>:\%(\%(^\||\)\s*\)\@<=\<en\%[dif]\>,' ..
+ \ '{:},' ..
+ \ '\<try\>:\%(\%(^\||\)\s*\)\@<=\<cat\%[ch]\>:\%(\%(^\||\)\s*\)\@<=\<fina\%[lly]\>:\%(\%(^\||\)\s*\)\@<=\<endt\%[ry]\>,' ..
+ \ '\<aug\%[roup]\s\+\%(END\>\)\@!\S:\<aug\%[roup]\s\+END\>,' ..
+ \ '\<class\>:\<endclass\>,' ..
+ \ '\<interface\>:\<endinterface\>,' ..
+ \ '\<enum\>:\<endenum\>'
please don't change the indentation.
>
" Ignore syntax region commands and settings, any 'en*' would clobber
" if-endif.
" - set spl=de,en
" - au! FileType javascript syntax region foldBraces start=/{/ end=/}/ …
" Also ignore here-doc and dictionary keys (vimVar).
let b:match_skip = 'synIDattr(synID(line("."), col("."), 1), "name")
- \ =~? "comment\\|string\\|vimSynReg\\|vimSet\\|vimLetHereDoc\\|vimVar"'
+ \ =~? "comment\\|string\\|vimSynReg\\|vimSet\\|vimLetHereDoc\\|vimVar"'
same here
> endif let &cpo = s:cpo_save unlet s:cpo_save " removed this, because 'cpoptions' is a global option. -" setlocal cpo+=M " makes \%( match \) +" setlocal cpo+=M " makes \%( match \)
same here
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you commented.
dkearns
dkearns left a comment (vim/vim#16680)@tpope, do you have any objections to a copy of scriptease#helptopic being included in the vim ftplugin?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you commented.
Enno
@Konfekt commented on this pull request.
> @@ -48,7 +49,49 @@ setlocal fo-=t fo+=croql
setlocal isk+=#
" Use :help to lookup the keyword under the cursor with K.
-setlocal keywordprg=:help
+" Distinguish between commands, options and functions.
+if !exists("*" .. expand("<SID>") .. "Help")
+ function s:Help(topic) abort
+ let topic = a:topic
+
+ if get(g:, 'syntax_on', 0)
+ silent! let syn = synIDattr(synID(line('.'), col('.'), 1), 'name')
It will return an empty string, falling back to :help if it fails; not worth bothering long-term users with potential error messages
—
Reply to this email directly, view it on GitHub.
You are receiving this because you commented.
Enno
> endif let &cpo = s:cpo_save unlet s:cpo_save " removed this, because 'cpoptions' is a global option. -" setlocal cpo+=M " makes \%( match \) +" setlocal cpo+=M " makes \%( match \)
Why the mix of tabs and spaces? This was not intentional, just a recurrent headache when contributing.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you commented.
Christian Brabandt
@chrisbra commented on this pull request.
> @@ -48,7 +49,49 @@ setlocal fo-=t fo+=croql
setlocal isk+=#
" Use :help to lookup the keyword under the cursor with K.
-setlocal keywordprg=:help
+" Distinguish between commands, options and functions.
+if !exists("*" .. expand("<SID>") .. "Help")
+ function s:Help(topic) abort
+ let topic = a:topic
+
+ if get(g:, 'syntax_on', 0)
+ silent! let syn = synIDattr(synID(line('.'), col('.'), 1), 'name')
and why would this fail when syntax highlighting is enabled?
—
Reply to this email directly, view it on GitHub.
You are receiving this because you commented.
Enno
@Konfekt commented on this pull request.
> @@ -48,7 +49,49 @@ setlocal fo-=t fo+=croql
setlocal isk+=#
" Use :help to lookup the keyword under the cursor with K.
-setlocal keywordprg=:help
+" Distinguish between commands, options and functions.
+if !exists("*" .. expand("<SID>") .. "Help")
+ function s:Help(topic) abort
+ let topic = a:topic
+
+ if get(g:, 'syntax_on', 0)
+ silent! let syn = synIDattr(synID(line('.'), col('.'), 1), 'name')
It's just a precautionary measure to exclude unpredicted failures
—
Reply to this email directly, view it on GitHub.
You are receiving this because you commented.
dkearns
@dkearns commented on this pull request.
> @@ -48,7 +49,49 @@ setlocal fo-=t fo+=croql
setlocal isk+=#
" Use :help to lookup the keyword under the cursor with K.
-setlocal keywordprg=:help
+" Distinguish between commands, options and functions.
+if !exists("*" .. expand("<SID>") .. "Help")
+ function s:Help(topic) abort
+ let topic = a:topic
+
+ if get(g:, 'syntax_on', 0)
+ silent! let syn = synIDattr(synID(line('.'), col('.'), 1), 'name')
I can't see how that can fail.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you commented.
dkearns
> endif let &cpo = s:cpo_save unlet s:cpo_save " removed this, because 'cpoptions' is a global option. -" setlocal cpo+=M " makes \%( match \) +" setlocal cpo+=M " makes \%( match \)
The runtime files were, for the longest time, supposed to be ts=8 noet. That's probably been relaxed.
I think we generally prefer dedicated whitespace fix commits if making those changes but I don't really think they're warranted. How about adding the modeline?
Please add yourself as a contributor, if you'd like.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you commented.
Enno
@Konfekt commented on this pull request.
> endif let &cpo = s:cpo_save unlet s:cpo_save " removed this, because 'cpoptions' is a global option. -" setlocal cpo+=M " makes \%( match \) +" setlocal cpo+=M " makes \%( match \)
Which modeline, the one settingts=8 noet or sw=2 et ?
—
Reply to this email directly, view it on GitHub.
You are receiving this because you commented.
Enno
@Konfekt pushed 2 commits.
You are receiving this because you are subscribed to this thread.
Enno
> @@ -48,7 +48,22 @@ setlocal fo-=t fo+=croql setlocal isk+=# " Use :help to lookup the keyword under the cursor with K. -setlocal keywordprg=:help +" Distinguish between commands, options and functions. +function! s:Help(args)
I guess we can go without finishing here
—
Reply to this email directly, view it on GitHub.
You are receiving this because you commented.
Christian Brabandt
chrisbra left a comment (vim/vim#16680)I am adding the modeline sw=2 et and update the header. Thanks all!
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you commented.
Christian Brabandt
—
Reply to this email directly, view it on GitHub.
You are receiving this because you commented.