Open jam1015 opened 1 month ago
I'll have time to review this tonight
Hi @jam1015
While I'm happy to delegate the code for neovim
, I'm concerned about any changes to the "core".
It seems like this is backward compatible.
Is there no way forward without touching the core? (if not, I'll need to spend more time thinking about this)
Hi! Thanks so much for taking the time to review it.
I tried for a long time to not touch the core but to make it work like it in my opinion should I had to.
Consider slime#send
before my changes:
function! slime#send(text)
call s:SlimeGetConfig()
" this used to return a string, but some receivers (coffee-script)
" will flush the rest of the buffer given a special sequence (ctrl-v)
" so we, possibly, send many strings -- but probably just one
let pieces = s:_EscapeText(a:text)
for piece in pieces
if type(piece) == 0 " a number
if piece > 0 " sleep accepts only positive count
execute 'sleep' piece . 'm'
endif
else
call s:SlimeDispatch('send', b:slime_config, piece)
endif
endfor
endfunction
If something goes wrong in s:SlimeGetConfig
and is caught in the functions in the targets, it still moves ahead and tries to send. In my Neovim code as it is in your version or the repo now, a nice error message is printed if the user misconfigures, slime#send()
but it tries to dispatch the target send function anyway, and a second error message is printed, in the case of Neovim indicating an empty config; the code sets the config to an empty dictionary when there is an error in configuration. This is confusing to the user; they just put in a configuration, why is it empty all of the sudden? Just one error message describing what went wrong is better.
with my changes:
function! slime#send(text)
if s:SlimeDispatchValidate("ValidEnv")
try
call s:SlimeGetConfig()
catch \invalid config\
return
endtry
" this used to return a string, but some receivers (coffee-script)
" will flush the rest of the buffer given a special sequence (ctrl-v)
" so we, possibly, send many strings -- but probably just one
let pieces = s:_EscapeText(a:text)
for piece in pieces
if type(piece) == 0 " a number
if piece > 0 " sleep accepts only positive count
execute 'sleep' piece . 'm'
endif
else
call s:SlimeDispatch('send', b:slime_config, piece)
endif
endfor
endif
endfunction
If the user misconfigures, the target configuration function prints an explanatory warning, throws 'invalid config' and it is caught in the core function, and the core function returns, and nothing gets sent. A single error message, whatever is printed by the code that did the throwing is printed and the user sees exactly what went wrong.
I can probably test more and think of more examples but they are all similar to this.
There is also the general benefit of cleaner code in the neovim target; each function has one purpose, not a purpose + validation.
These benefits could be extended to other targets; yes I know they all have much less code than the neovim target, but a target could come along one day for which these changes are helpful.
error 117 function does not exist
, to ensure backward compatibility. I know that relying on try-catch (or exceptions or something of the like) frowned upon in some programming languages like c++ but that is the best I can do here. See the comment in the body of function! s:SlimeDispatchValidate(name, ...)
for a justification. Hi @jam1015
Let me do another pass at this when I have more time.
In PR #419 I end my explanation with:
This PR has the desired changes that I talked about.
Currently functions in the neovim target deal with what they are supposed to do, and also with validation. This PR splits up those functions, making dedicated validation functions in the neovim target and putting the logic of how to call them in
autoload/slime.vim
. These changes inautoload/slime.vim
are compatible with targets that don't have the validation functions I added to Neovim, and the code for those targets could be expanded in the future to include validation functions. I've also confirmed that code from PR #424 is preserved, and that the problems identified in issue #427 don't resurface.Besides cleaning the code, the improvement is that if someone misconfigures the neovim target, an accurate warning message of just one line is displayed. previously only the 'terminal not found' warning message was displayed, multiple times for the same error. This is because the 'try catch' blocks in
autoload/slime.vim
stops multiple errors from firing.