Unset invalid slime_config since neovim terminal closed

jiz4oh commented 5 months ago

This pr basically is some improvements of https://github.com/jpalardy/vim-slime/pull/411

They are

Unset invalid slime_config since neovim terminal closed
Re-require config if b:slime_config is empty due to there is no way to unlet b:slime_config of a specified buffer
Do not raise E121 undefined variable error if there is no opened terminal

jpalardy commented 5 months ago

Hi @jiz4oh

I'll keep an eye on the thread, but I think you can discuss with @jam1015

jiz4oh commented 5 months ago

hi @jam1015, very interesting in what you do, and I had implemented some similar method. like below

function! s:reset(map) abort
  if exists('*s:check') 
    call s:check()
  endif

  if exists('b:slime_config') && empty(get(b:, 'slime_config')) 
    unlet b:slime_config 
  endif
  return a:map
endfunction

let g:slime_no_mappings = 1
xmap <silent><expr> gz <SID>reset('<Plug>SlimeRegionSend')
nmap <silent><expr> gz <SID>reset('<Plug>SlimeMotionSend')
nmap <silent><expr> gzz <SID>reset('<Plug>SlimeLineSend')
nmap <silent><expr> gZ <SID>reset('<Plug>SlimeMotionSend$')

augroup vim-slime-augroup
  autocmd!

  if has('nvim')
    let g:slime_target = 'neovim'

    function! s:check() abort
      let jobid = get(get(b:, 'slime_config', {}), 'jobid')
      if index(get(g:, 'slime_died_channels', []), str2nr(jobid)) != -1
        call setbufvar(bufnr(), 'slime_config', {})
      endif
    endfunction

    let g:slime_channel_mapping = {}
    let g:slime_died_channels = []

    autocmd TermOpen * let g:slime_channel_mapping[expand('<abuf>')] = &channel
    autocmd BufDelete term://* call add(g:slime_died_channels, get(g:slime_channel_mapping, expand('<abuf>'), v:null))
  elseif exists('##TerminalWinOpen')
    let g:slime_target = 'vimterminal'

    function! s:check()
      let bufnr = get(get(b:, 'slime_config', {}), 'bufnr')

      if !bufexists(bufnr)
        call setbufvar(bufnr(), 'slime_config', {})
      endif
    endfunction
  endif
augroup END

more detail can be found here

I referenced your implementation and made some improvements based on it, hope you like it!

jam1015 commented 5 months ago

hi @jiz4oh thanks so much for these contributions.

I actually take a very similar approach in a different branch validate_config that I have on my fork of the repo that I want to PR soon.

The approach it takes is to add the functions slime#targets#neovim#ValidEnv() and slime#targets#neovim#ValidConfig(config), and then modify functions in autoload/slime.vim to call them. The difference between ValidEnv and ValidConfig is that ValidEnv checks things that no changing the configuration could fix, whereas ValidConfig checks whether the config is valid, and can do the same things as ValidEnv as well for redundancy.

The changes to autoload/slime.vim are designed to be backwards compatible for targets that don't [yet] have the validation functions. I made this a separate PR from my other one because since it modifies core functions and not just Neovim target functions @jpalardy will apply much greater scrutiny to the changes.

I like my way of doing things because it allows the putting of all the validation capabilities in well-defined places for each target, rather than spread out. This can make it easier to implement validation both for the targets that already exist and for any new ones that might be added in the future.

I concede that it might be the case that that all this validation isn't necessary for other targets because we see here that @jpalardy was surprised at all the errors when the Neovim target wasn't configured right; I just supposed it was similar for all targets.

A rough to-do list of what I would do even if you didn't make this PR:

test vim-slime both on validate_config and on main for other targets to make sure of backwards compatibility, and see how helpful more validation might be for other targets. I especially care about the regular Vim terminal target because it is always available.
write out all the changes in detail similar to how I did for my previous major PR (not the one in response to your comments). I'm not good at looking at the diff in Github, the way it lists the changes doesn't make sense; maybe I can reformat, but it is easier to see the differences when I explain them function by function.

Ok in the meantime, if you see this before I do any of that, I think the best thing might be to close this PR, then look at my branch validate_config ; let me know what can be improved there; open a PR for that branch on my fork; we can agree on what to include/exclude over there; Some of what you include here can be added over there, but some of it might be already implemented. Once we've agreed that branch is as good as it can be we can do a PR on that branch to main on this repo.

jpalardy commented 5 months ago

Just chiming in ⬆️

Before any sweeping changes, especially in the common logic, let's have a chat first.

I don't think anybody would appreciate spending a bunch of time of things I might not want to merge.

jiz4oh commented 5 months ago

agreed @jpalardy, do you mind me open a new pr that only contain the Re-require config if b:slime_config is empty due to there is no way to unlet b:slime_config of a specified buffer feature? It's the only thing I cann't implement by my customization.

jpalardy commented 5 months ago

@jiz4oh

Open a PR — I think my general guidelines are:

changes in target-specific are welcomed (especially since I personally only use the tmux one)
changes in the "core" must pull their weight (i.e. let's not change things for aesthetics)
before investing too much, send a draft PR to discuss "the plan" — proportional the number of changes / lines of codes …
consider how changes might affect other people

jpalardy / vim-slime

Unset invalid slime_config since neovim terminal closed #416