krischik / vim-ada

Complete Ada-Mode as Vim-Ball or Tar.bz2
http://www.vim.org/scripts/script.php?script_id=1609
GNU General Public License v3.0
11 stars 3 forks source link

Typing al in any buffer gives an error #13

Closed TamaMcGlinn closed 2 years ago

TamaMcGlinn commented 2 years ago

After recently updating vim-ada, and trying to type a commit message containing the word 'all', I was given this error:

E107: missing parentheses: AdaLines

It is bad practice for a plugin to remap keys without explicit config in the user's vimrc, especially for anything in insert mode.

TamaMcGlinn commented 2 years ago

same goes for ar, av, ac, ao etc. You realise this makes it almost impossible to even type anything, right?

krischik commented 2 years ago

The mapping should only active if g:mapleader is defined — a technique I copied from nerdcommenter. Lets see:

      if exists('g:mapleader')
     execute "nnoremap <unique>" . l:mapping .      " :" . a:Command . "<CR>"
     execute "inoremap <unique>" . l:mapping . " <C-O>:" . a:Command . "<CR>"

Well, I'll have to do some further testing this to see if there are problems with the use of mapleader. There might be a bug in line 478.

dkearns commented 2 years ago

Have you got mapleader set to <space> by any chance?

krischik commented 2 years ago

There were a few bugs in setting up command, menu and key mappings. Those have been fixed. However the design was correct. No key mappings are made when g:mapleader is not defined. The observed behaviour would suggest that g:mapleader was defined but empty. g:mapleader should either be undefined or set to a key unused in insert mode — for example function key.

I added an error if g:mapleader is defined and empty to warn user that this is be erroneous.

TamaMcGlinn commented 2 years ago

After briefly being fixed in 5.3.1 this issue is back as of 5.3.2. My g:mapleader is defined as space for my own purposes but that doesn't mean I want these insert mode mappings. You are correct that NERDCommenter makes the same mistake. Don't follow their example.

krischik commented 2 years ago

The use of g:mapleader is an at least 15 year old established convention. You fighting the tools by setting g:mapleader to a space. No Fix.

However, you might want to read up on Enhancement #19 which is the fix done for 5.3.2. I am open to implementing actual standards if they are requested and prove is provided that it is indeed an established standard.

PS: I already did followed NERDCommenter's example 15 years ago when I first started to use g:mapleader.

TamaMcGlinn commented 2 years ago

I have been using mapleader for a long time, and never had a problem with the other 120 plugins I am currently using, or any that I have used in the past. The entire SpaceVim community uses space as their leader, so I don't believe this is very uncommon either. It is, in my view, a very clear rule in vimplugins, not to define mappings unasked. It is also an unwritten rule not to define mappings in insert mode unless absolutely necessary for their functionality. Finally, there is absolutely no reason that vim-ada should define mappings in non-Ada filetypes.

You are, of course, free to disagree. I just wanted to point out that this is going to be very off-putting for most vim users.

The normal way to do this in vimplugins is to create commands or functions, and put a section in the readme with example mappings, so that the user can choose to add these to their vimrc. Some arbitrary examples of plugins that do this correctly: fzf.vim harpoon gprselector vim-codefmt cheat.sh-vim vim-signify vimspector vim-maximizer. If you look at any of those readme's you will see which commands it exposes to the user, sometimes an explicit example for mapping it, and nary a note on using mapleader.

vim-easymotion is a sort of counter-example; it used to define mapleader like you do, but their readme now has this note: "The default leader has been changed to LeaderLeader to avoid conflicts with other plugins you may have installed." Obviously, this is not a generally good way of doing things; what happens when arbitrary plugin number 2 in your config decides on the same solution? Vim plugins should be standalone and never be able to conflict with each other, and this is easily achieved by not defining mappings (except on buffer types defined by the plugin). However, note that even they don't commit the cardinal sin of defining insert mode mappings, which, even when not used, will add a delay whenever you type any prefix of any mapping!

Another kind-of counterexample is found in e.g. vim-fugitive - but a key difference is that keybindings are only defined both in normal mode, and in buffer types defined by the plugin in the first place, such as gitcommit.

In summary, when faced with a choice of:

If you like, I can make this into a PR.

dkearns commented 2 years ago

Finally, there is absolutely no reason that vim-ada should define mappings in non-Ada filetypes.

The ftplugin mappings should be buffer-local and undone via the standard b:undo_ftplugin mechanism. See #21.

However, this is less clear for the mappings created from the compiler plugins as :make need not be applied to the current buffer.

The normal way to do this in vimplugins is to create commands or functions, and put a section in the readme with example mappings, so that the user can choose to add these to their vimrc.

It is 'standard' to define mappings in ftplugins and in particular to redefine standard keys to behave in a manner more appropriate for the given filetype.

krischik commented 2 years ago

@TamaMcGlinn So you did not check was done in #19 — because now you could just disable the mappings.

I like to remind you that I'm not getting payed for this plugin. I'm not an AdaCore employee and this plugin is first and foremost for my personal use.

If you like, I can make this into a PR.

I welcome pull request — unless it would break my way of using the plugin or disable functionality I use. Which was the case with the last pull request you send me. Which was more than off-putting.

When I restarted vim-ada I carefully checked Insights/Network to check for any updates and created pull requests to merge them in.

For krischik/vim-rainbow I pulled changes from four different users together. Carefully making sure nothing is lost in the process. That all functionality is keep so that those four users can merge my code with confidence.

I also mentioned everybody involved in the file header. I'm no stranger to accepting pull requests.

TamaMcGlinn commented 2 years ago

I'm sorry for stepping on your toes with the earlier PR. My intent was just to offer a quickfix, in case you didn't have time to fix it right now. I was trying to be friendly.

I am very grateful for your efforts in merging PRs from other authors. Checking Insights/Network is a real pain; perhaps ActiveForks is what you need.

19 is great, I would only invert it, so that by default we don't define mappings.

I also wonder; do you really use the bindings in insert mode?

krischik commented 2 years ago

I also wonder; do you really use the bindings in insert mode?

I wonder that as well. In some plugins they are helpful but maybe for the Ada Bundle not so. I'm going to observe my own behaviour in the coming weeks.

dkearns commented 2 years ago

I think insert mode mappings that perform tasks unrelated to inserting text are, at least, unusual.

TamaMcGlinn commented 1 year ago

Oh dear. I thought I had fixed this quite simply by uninstalling the plugin, but the NeoVim authors seem to have bundled this plugin by default now. Now it's everybody's problem.

i  <Space>al   *@<C-O>:call ada#List_Tag ()                                                                                                                                                                                                                                       
        Last set from /tmp/.mount_nvimKRnKhQ/usr/share/nvim/runtime/autoload/ada.vim line 592

:-1:

TamaMcGlinn commented 1 year ago

for anyone else encountering this, the fix is to switch to my fork, e.g.:

Plug 'TamaMcGlinn/vim-ada'

also fixed the Wiki issue in that. If anyone actually wants default mappings, or insert mode mappings, please let me know in an issue on that fork, then I would clean it up and make those available. For now it seems fine to just remove mappings altogether.

dkearns commented 1 year ago

A quick survey of the runtime files on my system suggests that insert-mode mappings are almost exclusively used for mapping unprintable characters to commands or for transforming/expanding inserted text.

I actually wasn't able to find any other examples mapping <Leader>[printable chars] in insert mode.

It appears that some of the most popular general configuration packages other than SpaceVim also default to using <Space> for the leader key. E.g., AstroNvim

There's also a related issue in the Vim repo from 2017.

I think these insert-mode mappings cause more trouble than they're worth and are probably difficult to understand the first time they strike for an unsuspecting user. I agree they should, at least, go behind an enabling configuration flag.