Main to merge

[jam1015]

In PR #419 I end my explanation with:

If you accept this, my further desired changes to autoload/slime.vim will allow this code to be simplified quite a bit.

Right now are also some double warning messages, and corner cases where you are prompted to config twice if you misconfigure the first time, and these are also fixed by my modifications to autoload/slime.vim and other files that I have in another branch.

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 in autoload/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.

[jpalardy]

I'll have time to review this tonight

[jpalardy]

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)

[jam1015]

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.

• A criticism that I do have of my code is that it uses a try catch block to catch 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.

[jpalardy]

Hi @jam1015

Let me do another pass at this when I have more time.

[jpalardy]

good stuff — let's see how it goes 👍