callback.on_exit needs three arguments to work properly, even if third is not used

[coachshea]

The job.on_exit requires a function that takes three arguments, or it will throw and error and not include the attachments.

As a neovim user, who has long been an ardent vifm user, my only painpoint in switching to neovim was that this plugin didn't work properly. I discovered vifm/neovim-vifm and began happily using it in place of this plugin. I then discovered that when editing vifm files, I lost my ftdetect and syntax. I offered a pull request to fix this, but of course, having two plugins trying to keep the syntax highlighting in sync carried potential problems. It was suggested to me by @xaizek to include both plugins and simply set let g:loaded_vifm = 'disable' While this initially appeared to work, I had a separate issue, by setting that to 'disable' g:vifm_exec and g:vifm_exec_args were not set, so the mail plugin didn't work. I set those by hand and then discoved the issue for which this pull-request addresses. While this pull-request will fix the issue for me, it does mean that new adopters, who might be new to vifm, but already using neovim, will need to know to include both plugins, disable this one, and set g:vifm_exec by hand.

If I might make a suggestion that would make life easier for all adopters. Simply combine the plugins. Move (almost) all you the logic currently in this plugin/vifm.vim to a new folder (i.e. src/vim_vifm.vim), take the logic from neovim-vifm and put it in the same folder (i.e. src/neovim_vifm.vim). and the plugin/vifm.vim file would simply contain the following:

if exists('loaded_vifm')
  finish
endif
let loaded_vifm = 1

if !exists('g:vifm_exec')
  let g:vifm_exec = 'vifm'
endif

if !exists('g:vifm_exec_args')
  let g:vifm_exec_args = ''
endif

if has('nvim')
  source src/neovim_vifm.vim
else
  source src/vim_vifm.vim
endif

Now, there will be one plugin to maintain. Maintaining those files will be no more difficult than they are now and changes to ftdetect, syntax, or any files in ftplugin will be automatically included for vim and neovim users. I'd be more than happy to help set this up as a pull request if desired.

Thank you for your time. I hope these suggstions and this pull-request help.

[xaizek]

Thanks for the fix!

" Work around handicapped neovim...

I like that the fix was needed below this line :)

The reason it's there is because they changed the way :! works in Vim, thus breaking plugins. There were two arguments in that callback not because the third one isn't used, but because it used to not exist, then they changed callback to require three arguments and broke the plugin.

Merging plugins isn't really appealing because they are too different, you'll essentially get two plugins with different interfaces, behaviour and documentation artificially combined into one. By the way, this one also works in neovim (well, after I'll fix the same issue with the callback in the plugin itself).

[coachshea]

I'm glad you like the fix. I respect your decision not to want to combine the plug-ins. You are right, they are two completely different interfaces. The README would be a nightmare to combine them. On the other hand, I still find it counter-intuitive to need two two plug-ins and disable one to make it all work together. Of course, I'll respect whatever decision you make, and thanks again for the great work you do with these plug-ins, but if I'm not going to be combined, I would still suggest "sharing" files between the two. I didn't realize at the time, that the same person keeping up both projects. In that scenario, it shouldn't be too hard to know that anytime you make an adjustment to one of the "shared" files, you need to copy it into the other plugin. That way, either one would work "out-of-the-box". In fact, doing it that way, you would not need to have "if has('nvim')" littered throughout the code, just simply make the appropriate adjustments where needed. That's my two cents. Whatever you decide, I won't bother you anymore. On the other hand, whichever way you do decide to go, if there is something I can do to help, please let me know. I love vim/nvim and I love vifm, so I really enjoy working on this.

-- John Shea Head Football Coach Spaulding High School Cell: 617-605-5763 Email: coachshea@fastmail.com

On Jan 1, 2018, at 3:22 PM, xaizek notifications@github.com wrote:

Thanks for the fix!

" Work around handicapped neovim...

I like that the fix was needed below this line :)

The reason it's there is because they changed the way :! works in Vim, thus breaking plugins. There were two arguments in that callback not because the third one isn't used, but because it used to not exist, then they changed callback to require three arguments and broke the plugin.

Merging plugins isn't really appealing because they are too different, you'll essentially get two plugins with different interfaces, behaviour and documentation artificially combined into one. By the way, this one also works in neovim (well, after I'll fix the same issue with the callback in the plugin itself).

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or mute the thread.

[xaizek]

I still find it counter-intuitive to need two two plug-ins and disable one to make it all work together.

Well, this wasn't intended, I didn't actually think about those issues with syntax highlight until you mentioned them.

I didn't realize at the time, that the same person keeping up both projects.

That's not the case, otherwise this one would support Vim only. neovim-vifm is a project of @rbong, I might answer on that plugin if rbong doesn't and it's something general.

I would still suggest "sharing" files between the two

That would make three repositories to contain the same files... This one is actually a mirror of data/vim directory of vifm/vifm to allow installation via various Vim plugin managers.

A better way might be extracting common part into third (vifm-runtime) plugin, but then it will be even more cumbersome to handle.

In general, I recognize the issue, I just don't see satisfactory resolution at this point.

[coachshea]

A better way might be extracting common part into third (vifm-runtime) plugin, but then it will be even more cumbersome to handle.

I considered suggesting that, but also realized the difficulties that you mentioned.

In general, I recognize the issue, I just don't see satisfactory resolution at this point.

I understand that. I've got it working the way that I want now, so I'm happy. Thank you so much for taking this much time, I really appreciate it. As mentioned previously, if there is anything that I can do to help, let me know.