preservim / nerdcommenter

Vim plugin for intensely nerdy commenting powers
Creative Commons Zero v1.0 Universal
4.98k stars 447 forks source link

NERDCommenter stopped loading commands #471

Closed gerardbm closed 3 years ago

gerardbm commented 3 years ago

Since the commit dec06b1fb9f529a34a3076d5b221b1fa6afd5967, NERDCommenter is not loading the commands, so I had to install the plugin from the previous commit, like this:

Plug 'scrooloose/nerdcommenter', { 'commit': 'a5d1663' }
alerque commented 3 years ago

Sorry to hear you are having issues. We'd love to get whatever the trouble is resolved again for you (and potentially others). That being said I don't see what the problem is. I and several other people have confirmed that the current changes do work. In dbc631a (not dec06b1) there was a major reorganization and changes to how the plugin loads, but it should also be a drop in replacement.

First (and this is not your real problem) I'll note that the repository location has changed years ago now. Your line should be:

Plug 'preservim/nerdcommenter'

Now on to what the real problem might be... and here I'm clueless because by default it should just load when needed as usual. The bindings and commands are defined as usual in the same file, only the actual code to run some of them has been moved to an autoloader that fetches them on first use (see #376).

What exactly do you mean by "not loading the commands"? If you call them what error do you get?

@antonk52 any ideas on this one?

alerque commented 3 years ago

See also #472, which seems to be the same bug. Clearly there is some use case out there that is different than my own (which still works fine).

@Penaz91 Are you sure your ability to :call <function> is gone, or is the tab complete for currently loaded things all that is missing?

Penaz91 commented 3 years ago

The call NERDComment('n', 'Sexy') is done via script and returns the following error:

Vim(call):E117: Unknown function: NERDComment

Also the tab completion is not working either.

gerardbm commented 3 years ago

I was gonna comment the same, the error returned is the following:

E117: Unknown function: NERDComment

That occurs after cleaning my setup, as well.

Penaz91 commented 3 years ago

It seems that the NERDComment function has been replaced by nerdcommenter#Comment

Edit: After modifying my scripts to use the new function, seems everything is back in working order.

I can see from the commits that the interfaces changed like follows: NERDComment -> nerdcommenter#Comment NERDCommentIsLineCommented -> nerdcommenter#IsLineCommented

I can't find a corresponding for IsCharCommented

alerque commented 3 years ago

I think there should probably be a shim function (optionally that throw deprecation warnings) so that the old names can be called and trigger the autoload on first run. It seems that got done for some of the more specific commands already, just missing the generic one. Still looking into this.

antonk52 commented 3 years ago

Thank you for the issue, this was somewhat expected to break.

A potential fix to preserve complete backward compatibility would be to define functions with the same name in plugin/.vim which in turn would call the autoloaded functions ie

function NERDComment()
    return nerdcommenter#Comment()
endfunction

This way the older called functions would still be accessible in globally but would only load the actual code once it is needed.

As for the IsCharCommented, it seems that it was lost in #376 since the earlier commits seem to include it. This should be an easy copy paste from one of the earlier commits.

Penaz91 commented 3 years ago

Thank you very much for your help.

I also think that it would be a good idea to update the help file to reflect the new available interfaces.

alerque commented 3 years ago

Yes that's exactly the sort of shim function I was thinking of. Lets get that in ASAP and then think about how to also add a warning message to it so people know they need to update their own code. For now lets fix the breakage. Are you working on a PR or should I?

antonk52 commented 3 years ago

I won't be able to get around to create a PR until the weekend, if you have time before that feel free to create the PR

alerque commented 3 years ago

@Penaz91 or @gerardbm Any chance you can check out #473 and make sure those fixed work for you? I'm pretty sure that's all the functions that needed shimming since the other ones were (and still are) created with a dynamic map system.

I'll merge soon if I don't hear anything, but I'll poke the docs a bit while I wait for confirmation.

Penaz91 commented 3 years ago

It seems that the shim function NERDComment doesn't handle ranges correctly, while the nerdcommenter#Comment does.

More precisely, the function I use does execute "0,8call NERDComment('n', 'Sexy')" which should wrap the first 8 rows in a single "block comment", while the shim function puts each one of the first 3 lines on a block comment by itself.

alerque commented 3 years ago

@Penaz91 Interesting catch. I can't figure out how to pass range data through at all. I can do it for a command calling a function (where <f-args> takes care of it) but I can't figure it out for a function calling a function. I've added a deprecation message to the shims and added detection of the range case and upgraded it to an error so at least the failure mode is sensible and people have a clear upgrade path.

I'm tempted to merge that and work on the docs as I can because some shim is better than no shim. What do you say?

Penaz91 commented 3 years ago

Surely a simple shim is better than none, I'm looking around to see how it would be possible to get ranges.

Maybe it's possible possible to memorize the range by using line("'<") and line("'>")?

In case it's not possible, we can still use this shim to "force" people to migrate to the new syntax.

alerque commented 3 years ago

As far as I could find couldn't pass the whole a: or any extra values without messing up the expected arguments of the function in the autoload section. Of course we could setup a parallel function with different arguments or maybe add optional extra arguments, but that seems to be starting to defeat the purpose of setting up the autoload in the first place.

I'd be happy to accept a contribution that makes a more effective shim. For now knowing it is broken for many people I'm going to get us started with this one.