jpalardy / vim-slime

A vim plugin to give you some slime. (Emacs)
http://technotales.wordpress.com/2007/10/03/like-slime-for-vim/
MIT License
1.83k stars 223 forks source link

Validate in target #419

Closed jam1015 closed 3 months ago

jam1015 commented 4 months ago

In my recent issue I proposed changes to autoload/slime.vim. You suggested a more conservative approach. The two approaches are summarized in this image: image With your preferred way on the left, and what I suggested on the right, with your annotations overlaid.

I said I'd open two pull requests, one with your preferred way and one with my preferred way. Instead I'm just opening this one, which is your preferred way; the changes to the Neovim target code are quite extensive (in a way that I feel is justified) so it will be easier to try to get this merged first, because the changes here are already quite extensive.

I know that this makes the Neovim target the one with the most code by far, but I think what I have here enhances the user experience overall. Give it a try/compare it to how it was before to sew why I made it this way.

Here is an explanation of the changes:

README.md

A small change to phrasing.

plugin/slime.vim

slime#targets#neovim#SlimeAddChannel(expand('<abuf>')) and slime#targets#neovim#SlimeClearChannel(expand('<abuf>')) use <abuf> now to track exactly the buffers being acted on regardless of whether they are active. I didn't know about this feature before and it allowed me to simplify these channel tracking functions/not have to deal with corner cases.

autoload/slime.vim

assets/doc/targets/neovim.md

A full explanation of the features I added to autoload/slime.vim (read it to see everything I have added). This is a lot but I commit to maintaining it.

The goal here is to give very clear documentation so that someone with late-beginner vim knowledge can get started more easily.

I found that these changes were difficult to implement using overrides, hence this PR.

autoload/slime/targets/neovim.vim

Here are the big changes.

Calls to my validation functions are added by way of s:protected_validation_and_clear which checks for existence of a buffer's config, and then makes sure that it is correct. If the config is not valid, all identical configs in other buffers are cleared.

slime#targets#neovim#config()

Wrapped in a call that checks for a valid environment.

Changed to allow config with a menu prompt or direct entry on the command line (configurable as detailed in the Neovim documentation).

Still allows users to enter pid or jobid.

I uset the temp_config variable so that any status line customization doesn't show any intermediate value of the config.

slime#targets#neovim#send

Wrapped into a a call to the validation function.

If the config is not valid, it is cleared, and all similar configs are cleared from other buffers.

SlimeAddChannel and SlimeClearChannel

Keep a master list of open terminals.

These functions are greatly simplified from where they were before, because of the use of <abuf> in the autocommands that call them.

ValidEnv

Checks if the environment is valid. Basically means if there is an open terminal or not.

ValidConfig

Checks if a config is valid. Has an option to be silent.

Last_channel_to_jobid and Last_channel_to_pid

Are menu completion functions for the calls to input in the configuration function.

clear_related_bufs and clear_all_bufs

Clear specified, or all, configs from all open buffers.

config_with_menu and buffer_dictionary_to_string

Provide menu based configuration rather than direct input.

Future Improvements

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.

jpalardy commented 4 months ago

Hi @jam1015

89 commits and ≈500 lines: it's going to take a bit, based on my schedule.

Let me see how it falls and I'll let you know.

jam1015 commented 4 months ago

There is probably some standard for commits tht would make the history easier to go through.

Will consider going back and artificially making a more logical commit history.

Thanks a lot for all the work you do!

jpalardy commented 3 months ago

Hi @jam1015

Sorry it took so long.

Let me merge this and we'll see how it goes 👍

Thanks for the PR