[vim/vim] Remove `complete_match()` and `'isexpand'`: redundant and misleading (Issue #18563)
girish
girishji created an issue (vim/vim#18563)Remove complete_match() and 'isexpand'
Summary
complete_match() and 'isexpand' add no real functionality to Vim.
They duplicate what strridx() already does, yet pretend to be part of the completion system.
Both should be removed to simplify the code and documentation.
I came across them through this comment: #18369 (comment)
Details
f_complete_match()ininsexpand.cdoes not call any completion code.
It’s just aSTRNCMP()wrapper with fluff logic.'isexpand'exists only as a proxy argument to that function.
It does nothing on its own and amounts to misuse of a new option.
The name “complete” implies integration with completion, but there is none.
The implementation is trivial string matching that belongs in Vimscript, not in C.
Proof
A pure Vimscript function reproduces the same behavior:
func CompleteMatch(triggers, sep=',') let line = getline('.')->strpart(0, col('.') - 1) let result = [] for trig in split(a:triggers, a:sep) let idx = strridx(line, trig) if l:idx >= 0 call add(result, [idx + 1, trig]) endif endfor return result endfunc
Replacing complete_match() and 'isexpand' in
/testdir/test_ins_complete.vim with this function passes Test_complete_match() unchanged.
That confirms these C implementations are unnecessary.
let $TEST_FILTER = 'Test_complete_match'
func Test_complete_match()
func CompleteMatch(triggers, sep=',')
let line = getline('.')->strpart(0, col('.') - 1)
let result = []
for trig in split(a:triggers, a:sep)
let idx = strridx(line, trig)
if l:idx >= 0
call add(result, [idx + 1, trig])
endif
endfor
return result
endfunc
" set isexpand=.,/,->,abc,/*,_
func TestComplete()
" let res = complete_match()
let res = CompleteMatch('.,/*,/,->,abc,_')
if res->len() == 0
return
endif
let [startcol, expandchar] = res[0]
if startcol >= 0
let line = getline('.')
let items = []
if expandchar == '/*'
let items = ['/** */']
elseif expandchar =~ '^/'
let items = ['/*! */', '// TODO:', '// fixme:']
elseif expandchar =~ '^\.' && startcol < 4
let items = ['length()', 'push()', 'pop()', 'slice()']
elseif expandchar =~ '^\.' && startcol > 4
let items = ['map()', 'filter()', 'reduce()']
elseif expandchar =~ '^\abc'
let items = ['def', 'ghk']
elseif expandchar =~ '^\->'
let items = ['free()', 'xfree()']
else
let items = ['test1', 'test2', 'test3']
endif
call complete(expandchar =~ '^/' ? startcol : startcol + strlen(expandchar), items)
endif
endfunc
new
inoremap <buffer> <F5> <cmd>call TestComplete()<CR>
call feedkeys("S/*\<F5>\<C-Y>", 'tx')
call assert_equal('/** */', getline('.'))
call feedkeys("S/\<F5>\<C-N>\<C-Y>", 'tx')
call assert_equal('// TODO:', getline('.'))
call feedkeys("Swp.\<F5>\<C-N>\<C-Y>", 'tx')
call assert_equal('wp.push()', getline('.'))
call feedkeys("Swp.property.\<F5>\<C-N>\<C-Y>", 'tx')
call assert_equal('wp.property.filter()', getline('.'))
call feedkeys("Sp->\<F5>\<C-N>\<C-Y>", 'tx')
call assert_equal('p->xfree()', getline('.'))
call feedkeys("Swp->property.\<F5>\<C-Y>", 'tx')
call assert_equal('wp->property.map()', getline('.'))
call feedkeys("Sabc\<F5>\<C-Y>", 'tx')
call assert_equal('abcdef', getline('.'))
call feedkeys("S_\<F5>\<C-Y>", 'tx')
call assert_equal('_test1', getline('.'))
" set ise&
" call feedkeys("Sabc \<ESC>:let g:result=complete_match()\<CR>", 'tx')
" call assert_equal([[1, 'abc']], g:result)
" call assert_fails('call complete_match(99, 0)', 'E966:')
" call assert_fails('call complete_match(1, 99)', 'E964:')
" call assert_fails('call complete_match(1)', 'E474:')
" set ise=你好,好
" call feedkeys("S你好 \<ESC>:let g:result=complete_match()\<CR>", 'tx')
call feedkeys("S你好 \<ESC>:let g:result=CompleteMatch('你好,好')\<CR>", 'tx')
call assert_equal([[1, '你好'], [4, '好']], g:result)
" set ise=\\,,->
" call feedkeys("Sabc, \<ESC>:let g:result=complete_match()\<CR>", 'tx')
call feedkeys("Sabc, \<ESC>:let g:result=CompleteMatch(\",->\", \",\\\\zs\")\<CR>", 'tx')
call assert_equal([[4, ',']], g:result)
" set ise=\ ,=
" call feedkeys("Sif true \<ESC>:let g:result=complete_match()\<CR>", 'tx')
call feedkeys("Sif true \<ESC>:let g:result=CompleteMatch(\" ,=\")\<CR>", 'tx')
call assert_equal([[8, ' ']], g:result)
" call feedkeys("Slet a = \<ESC>:let g:result=complete_match()\<CR>", 'tx')
call feedkeys("Slet a = \<ESC>:let g:result=CompleteMatch(\"=\")\<CR>", 'tx')
call assert_equal([[7, '=']], g:result)
" set ise={,\ ,=
" call feedkeys("Sif true \<ESC>:let g:result=complete_match()\<CR>", 'tx')
call feedkeys("Sif true \<ESC>:let g:result=CompleteMatch(\"{,\ ,=\")\<CR>", 'tx')
call assert_equal([[8, ' ']], g:result)
" call feedkeys("S{ \<ESC>:let g:result=complete_match()\<CR>", 'tx')
call feedkeys("S{ \<ESC>:let g:result=CompleteMatch(\"{,\ ,=\")\<CR>", 'tx')
call assert_equal([[1, '{']], g:result)
bw!
unlet g:result
set isexpand&
delfunc TestComplete
endfunc
Request
- Drop both from the core code.
- If compatibility is needed (I don't think so), re-implement them in Vimscript.
- Fix documentation to remove references suggesting they’re tied to completion.
This would reduce code size, remove dead weight, and make the API clearer.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Tomasz N
przepompownia left a comment (vim/vim#18563)At first glance: your explanation sounds convincing and seems to left a user more flexibility in distinguishing specific cases. For example I keep in mind explicit flag: "has the trigger string to be a part of completed word or not" (or even an explicit offset instead) to reduce the logic guessing the correct startcol. Instead of the (serialized) list of triggers we could use a dictionary indexed with them where each value contains such flag/offset, the possible results (maybe as a callback too) and another needed data. I need some free time to convert the above example into such form.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Maxim Kim
habamax left a comment (vim/vim#18563)
- If compatibility is needed (I don't think so), re-implement them in Vimscript.
Removing it would definitely break someone's code. I am not sure if Vimscript re-implementation would help with compatibility -- the function would need to have the same name which is impossible as vimscript function should start with upper case letter.
This would be a breaking change in either way (which I am fine with) and needs to be assessed by core team.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
zeertzjq
zeertzjq left a comment (vim/vim#18563)Related #16716 (review)
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
girish
girishji left a comment (vim/vim#18563)If compatibility is needed (I don't think so), re-implement them in Vimscript.
I'd change the above statement. Since complete_match() and isexpand have nothing to do with completion mechanism, best to take the L and nuke them. If anyone insists on compatibility, point them to the Vimscript version documented above.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
girish
girishji left a comment (vim/vim#18563)Related #16716 (review)
Given your understanding of the completion code, I’m surprised you didn’t flag that the PR shouldn’t have been merged.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Tomasz N
przepompownia left a comment (vim/vim#18563)To be precise: my comment is about the alternative shown above. I have no opinion on how to wisely deal with that existing feature.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Christian Brabandt
chrisbra left a comment (vim/vim#18563)Okay, so this comes from v9.1.1341 (commit bcd5995) from half a year ago. I'd be fine with reverting although this has been here for a little while. But like with the other similar issue, I'd prefer if @dkearns or @yegappan or @zeertzjq agree (or overrule me in my opinion). I don't want to make this decision alone.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Tomasz N
przepompownia left a comment (vim/vim#18563)Even if we have found an approach that seems to be better, thanks for @glepnir and other contributors for working on #16716!
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
zeertzjq
zeertzjq left a comment (vim/vim#18563)I didn't like this function when it was added in the first place, but didn't have a strong enough opinion against it.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Yegappan Lakshmanan
yegappan left a comment (vim/vim#18563)As many completion related features are recently added, I consider this to be still under development (till we make a major release). So I agree with reverting this. Instead of getting stuck with a sub-optimal or redundant features, it is better to clean them up now before the major release.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
dkearns
dkearns left a comment (vim/vim#18563)I’ll admit I haven’t been following completion developments as closely as I should have. However, I coincidentally stumbled over these the other day when adding some missing entries to the help. I found them confusing enough that I was also considering flagging them once I'd had a better look at the history. I'd be happy for them to be removed.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Christian Brabandt
Closed #18563 as completed via cbcbff8.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.