Proposal for Changes to `autoload/slime.vim`

Where We Are Right Now

You accepted my previous PR, #411 that improved terminal tracking in the Neovim target.

@jiz4oh then opened a PR, #416 that improved on it. I let him know that I had another branch that had implemented some of what he had done. I opened a PR of my branch into his branch that would then be incoroporated into PR #416.

Since doing that, in my own fork I've made more changes to the Neovim target, which merit their own PR.

The important thing is that my changes include some (as far as I can tell) backwards-compatible changes to autoload/slime.vim.

I'm opening this issue to present those changes to you and get your feedback before opening the full pull request with all my changes.

If you don't agree with these changes to autoload/slime.vim that's ok, I can work around it in in autoload/slime/targets/neovim.vim, it just won't be as clean.

Changes to `autoload/slime.vim`

Deleting redundant calls to `s:SlimeGetConfig`

The various send functions

slime#send slime#send_op slime#send_range slime#send_lines

all call s:SlimeGetConfig(). But all of them call slime#send so s:SlimeGetConfig() is called twice for each send, which is redundant. I got rid of those calls in all except for slime#send. Doesn't seem to break anything when I try several targets but you might have some insight on the purpose of this redundancy.

I know redundant can sometimes be a good thing (columns on a bridge) but in this case it can get in the way with the other changes I make (keep reading).

Adding a Modified version of `s:SlimeDispatch`

I add a new dispatch function:

function! s:SlimeDispatchValidate(name, ...)
  " allow custom override
  let override_fn = "SlimeOverride" . slime#common#capitalize(a:name)
  if exists("*" . override_fn)
    return call(override_fn, a:000)
  endif

  let fun_string = "slime#targets#" . slime#config#resolve("target") . "#" . a:name
  " using try catch because exists() doesn't detect autoload functions that aren't yet loaded
  " the idea is to return the interger 1 for true in cases where a target doesn't have
  " the called validation function implemented. E117 is 'Unknown function'.
  try
    return call(fun_string, a:000)
  catch /^Vim\%((\a\+)\)\=:E117:/
    return 1
  endtry

endfunction

The idea here is to be able to try to call a function that may be implemented in some targets but not others. I call it SlimeDispatchValidate because those functions that I've added to the Neovim target, which I use this function to dispatch, are validation functions. It returns 1, meaning logical true in this case, if the validation function is not implemented for the target. This means that the logic of the code will flow like it did before for such targets.

I could have just edited s:SlimeDispatch instead of making a new function but it, but 1 might not make sense as a default return value for all cases like it does in the case of a validation function.

ValidEnv/ValidConfig

The specific functions that I use SlimeDispatchValidate on are ValidEnv and ValidConfig for targets where they might be implemented, and which I have implemented in the Neovim target.

The purpose of ValidEnv and ValidConfig:

ValidConfig: Takes a config and returns 1 if it is valid, 0 if invalid, where valid means when used it will successfully send to a target.
ValidEnv: Checks if there are any valid targets available, in which case we can say there is a 'valid environment'. If the environment isn't valid it doesn't matter how you configure, there are no targes to send to. This function can perform a subset of the checks that ValidConfig does.

The other changes to this file are using these two functions to protect calls to to other functions.

Changes to `slime#config`

The slime#config function strictly has additions:

function! slime#config() abort
  call inputsave()
  if s:SlimeDispatchValidate("ValidEnv")
    call s:SlimeDispatch('config')

    if !s:SlimeDispatchValidate("ValidConfig", "b:slime_config")
      unlet b:slime_config
    endif
  endif
  call inputrestore()
endfunction

The whole body is wrapped in a check for a valid environment, and it checks that whatever config the user set is valid. If neither ValidConfig nor ValidEnv is implemented for a particular target it runs as it did before.

Changes to `slime#send`

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

The whole function body is wrapped in a call to ValidEnv, and then begins with a try/catch block call to SlimeGetConfig, and catches the exception that SlimeGetConfig is modified to throw if the config is not valid. This is the one call to SlimeGetConfig that remains in the send functions because I deleted them from the other ones. I can put them back if you want.

Changes to `SlimeGetConfig`

Calls to ValidConfig are added, including in place of exists("b:slime_config). If at the end of the function the config is not valid, the invalid config exception is thrown which is caught in the slime#send function.

function! s:SlimeGetConfig()
  " b:slime_config already configured...
  if exists("b:slime_config")
    if s:SlimeDispatchValidate("ValidConfig", b:slime_config)
      return
    endif
  endif
  " assume defaults, if they exist

  if exists("g:slime_default_config")
    let b:slime_config = g:slime_default_config
    if !s:SlimeDispatchValidate("ValidConfig", b:slime_config)
      unlet b:slime_config
    elseif exists("g:slime_dont_ask_default") && g:slime_dont_ask_default
      return
    endif
  endif

  " skip confirmation, if configured
  if exists("g:slime_dont_ask_default") && g:slime_dont_ask_default
    return
  endif

  " prompt user
  call s:SlimeDispatch('config')

  if s:SlimeDispatchValidate("ValidConfig", b:slime_config)
    return
  else
    unlet b:slime_config
    throw "invalid config"
  endif

endfunction

Why I Made These Changes

It makes validation in the targets simpler. With these changes not in place validation has to be spread through the send/config target functions. With these in place valdiation is called automatically when necessary and the code in the config/send target functions can just focus on that.

I notice that other targets have less code than what I've added to the Neovim target, which kind of mitigates the need for the changes I've made. It could be the case that, like you noticed, the Neovim channel system is prone to show errors and needs more guardrails set up to give the user a nice experience. Also these pre-placed calls to validation functions could be helpful for targets that may be added in the future.

The actual changes to the Neovim target include the validation functions, as well as a new feature and a bug fixes.

In any case I will include a thorough explanation like this.

I had another thought to add onto this but I forgot what it was.

Ok let me knoow if you are open to these changes to autoload/slime.vim and I will open a PR, if you are not I will work around it.

Also let me know if whatever PR I may open, you might prefer that it be broken into separate PRs for different features, or a PR for the changes described here and a PR for changes to the Neovim target.

hi @jam1015

I received the notification for this (I'm here), but I don't have a lot of bandwidth at this time.

Let me come back to this soon and give it my full attention.

Hi @jam1015

I read though all this, but it remains somewhat abstract.

Some context: back in 2007, I wrote this. I'm not even sure if that's the original code, because I remember it to be even shorter. It was so short you could paste it in your vimrc and start using it right away.

I think my original philosophy was: so little code, anybody can debug this. By extension, if you misconfigure or something goes wrong, you can fix it yourself. (in some way, that reminds me of Right to Repair)

What I would call "slime core" does very little. It generates some config and sends your text down a destination.

…

In my mind, there are 3 customization points in vim-slime:

how to generate config: which varies for each target, but also in complexity and what reasonable defaults would look like
how to send text to a target: obviously depends on the target, usually (and ironically) less complicated than config
text-processing pipeline: what, if any, changes need to be made to the text before sending to the target — varies by target, REPL, and language

vim-core

…

As you might have seen on the vim-slime-ext-plugins side of things, I think the solution is to allow the user to "plug" into these customization points

a simpler core, with overridable functions
but reasonable target-specific implementations

…

So, stepping back into your proposed changes:

why do we need to validate config? env? (how did we get there?)
why not let it crash? (how easy/not for the user to self-diagnose?)

I used to have a discord channel to discuss this — it might be an easier/faster medium to move this forward. Let me know.

discord: https://discord.gg/CTHR7wusVY (we'll try this as an experiment)

Hi! Thanks for your response, especially the insight into minimalist philosophy of the plugin. Of the three points of customization, my proposed changes focus on the configuration generation step. I'd say that I'm approaching my proposed changes with 'clear user feedback' as the main goal with modularity and minimalism as complementary goals.

Concretely, with my proposed changes, for a target with the validation functions implemented, your flow chart is expanded to look like:

slime_flow1_2

So to answer your two questions in reverse order:

Why not let it crash? Because it is generally a more pleasant user experience when errors are gracefully handled and useful feedback is given to the user. You noticed that, the way it is now, when the Neovim target is misconfigured, ugly looking errors pop up. Not a nice experience. On the other hand, it could fail 'silently'. Imagine you mis-type the configuration with a tmux pane as the target. You send, and nothing appears. Some useful feedback that can be provided is whether the text is being sent off to nowhere or if it is being sent to a pane in a different tmux socket running somewhere in the background. This feedback is definitely in service of helping the user fix things when something goes wrong.
Validating the environment and the config is the way to provide the useful feedback. In my branch (validate_config ) I implemented it as cleanly as I could, which meant making the backwards-compatible changes I in autoload/slime.vim as well as the Neovim target code. The changes allow each function in the target code to do it's job, without worrying about correctness (the config function just configures, doesn't have to worry if the config is making is valid, that gets checked by ValidConfig called elsewhere). This has the added bonus of letting users more easily make their own override functions for targets that don't have validation implemented in the target code yet.

I think these changes/the advantages will be more tangible if I open my PR and you try out the Neovim target again.

I'm happy to chat on Discord but I think these Github comments are well suited to their purpose.

Follow-up questions (based on your diagram)

is the environment valid?

does that evolve over time? can it be valid now and invalid later? (I think so, but I'd like to discuss circumstances)

is there a valid config?

same here — is that something that can be true now but false later?

…

Context:

I think we can shove most of the logic in 2 "boxes"

"configure" : handles "prompt user", "validate config", and "notify user"
"send to target" : does actually sending, but could sanity-check itself first — if things have changed, "panic" and re-trigger "configure"

This allows most plugins to stay as they are, while allowing for more flexibility. I tried to reconcile this using both diagrams.

revised-flow

Hi! to answer your questions:

1) Can the environment become invalid? The environment can definitely become invalid; if all target repls are closed the environment is not valid anymore, and no possible config can be correct. Whether it can be correct again later with no intervention differs by target; with Tmux it seems possible, because the same socket/session/pane/window combo can reappear, but with Neovim terminal it is not possible, because the same jobid will never reappear.

2) Can the config become invalid? Yes; The environment becoming invalid is a sub-case of the config becoming invalid. Also the user can run :SlimeConfig again and make a typo. Or some other script can wipe buffer variables including the config. Anything can happen.

I don't understand the annotations you've made on my diagram in yellow and blue, but that's ok. I can do it your way on the left, but as I mentioned, the code is slightly less elegant.

OK so here's what I'll do, unless you explicitly object: I'll open two PRs, one with the approach on the left, and one with the approach on the right (whose code is ready to go). You can merge your preferred version, or neither. I'll hopefully get to it in the next couple of days.

jpalardy / vim-slime