API availability and mappings

[mbenford]

Is there a reason some API methods are only available when some mappings are enabled? I guess many people (myself included) will use the API to define their own custom key mappings. It would be nice if one could disable all mappings and yet be able to use all API methods.

[numToStr]

Are you talking about these functions https://github.com/numToStr/Comment.nvim/blob/master/doc/API.md? Or do you want something else?

[mbenford]

Yeah, sorry for explaining it poorly.

I noticed I had to set mappings = {extras = true} in order to use require'Comment.api'.gcO(), but that ended up creating gcO as a mapping. It would be nice if I could set mappings = {} and yet be able to use all API methods.

[numToStr]

Ahh, I get it now. To be honest I did that on purpose as gc{o,O,A} can be easily re-created as you can see in the following example

local E = require('Comment.extra')
local U = require('Comment.utils')
local A = require('Comment.api')

local cfg = A.get_config() or {}

E.norm_o(U.ctype.line, cfg) -- gco
E.norm_o(U.ctype.block, cfg) -- (Bonus) gbo

E.norm_O(U.ctype.line, cfg) -- gcO
E.norm_O(U.ctype.block, cfg) -- (Bonus) gbO

E.norm_A(U.ctype.line, cfg) -- gcA
E.norm_A(U.ctype.block, cfg) -- (Bonus) gbA

(Only thing is that these modules aren't documented yet. But It's WIP 😃)

[mbenford]

Oh, I thought the goal of the newly released API was precisely to eliminate the need of such code.

Do you think it would be bad design if the whole API was fully available at all times?

[clason]

Seconded -- with Lua plugins, the API should be the primary export, with mappings only (optional) sugar on top.

(Here the basic mappings are so ubiquitous for the purpose that setting them by default is OK in my eyes.)

But: Such an API should use more descriptive names rather than just expose each mapping as an eponymous function, so the current approach isn't bad -- I expect that once they're documented, the problem will go away ;)

[numToStr]

Seconded -- with Lua plugins, the API should be the primary export, with mappings only (optional) sugar on top.

I am also figuring that out my myself how to export the functions nicely. I mean they are kinda exported via require('Comment.{api,opfunc,extra}') and you can even use them to make your custom commenting function. IMO the only thing that is missing is proper :help docs.

(Here the basic mappings are so ubiquitous for the purpose that setting them by default is OK in my eyes.)

Yeah, this was also my first observation as the community was already familiar with vim-commentary :)

[clason]

IMO the only thing that is missing is proper :help docs.

Agreed; although I would prefer toggle_line(vmode,cfg) for gcc (with toggle_line() using the right defaults to get the current toggle() behavior) -- have the name describe what it does, not the "usual mapping" (which people coming fresh to comment.nvim would not know).

What I don't think is a good idea is to gate API functions behind mapping setup -- if they are exposed they should always be exposed, since API functions can never conflict. (Or you don't provide them at all since they're redundant; that's fine, too.)

[mbenford]

@numToStr IMO you did an awesome job at creating/exporting an API so people can better use your plugin. The only thing that was unexpected to me was the need to enable extra key mappings in order for the API to be available to use. At best those key mappings will simply be ignored (my case); at worst they may clash with other mappings that might have been already set by the user.

[numToStr]

@clason @mbenford Thanks for bringing this up. I'll rethink the API situation a bit more and let you know. Meanwhile, If you have any suggestions please let me know :)

[numToStr]

Care to try #94 ;)

(I'll later update the docs for the APIs)

[mbenford]

@numToStr Thanks for the update. I think the new API methods are much more user-friendly!

I managed to setup my keymaps as follows:

require'Comment'.setup{
	mappings = {}, -- no need to enable anything. Yay!
	ignore = '^$',
}

local map = require'map'
map.n{'<leader>cc', '<cmd>lua require"Comment.api".toggleln_linewise_op()<cr>'}
map.n{'<leader>cb', '<cmd>lua require"Comment.api".toggleln_blockwise_op()<cr>'}
map.n{'<leader>c', '<cmd>lua require"Comment.api".call("toggle_linewise_op")<cr>g@'}
map.v{'<leader>cc', '<esc><cmd>lua require"Comment.api".toggle_linewise_op(vim.fn.visualmode())<cr>'}
map.v{'<leader>cb', '<esc><cmd>lua require"Comment.api".toggle_blockwise_op(vim.fn.visualmode())<cr>'}
map.n{'<leader>co', '<cmd>lua require"Comment.api".insert_linewise_above()<cr>'}
map.n{'<leader>ca', '<cmd>lua require"Comment.api".insert_linewise_eol()<cr>'}

It works like a charm. However, I had to peek into api.lua in order to understand how the plugin itself was calling some methods, and I found that some extra configuration is needed. For instance, .call must be used for operator-pending mode to work, which seems to be a private function, and <esc> must called for visual mode. I think it would be best if these details were abstracted away and a simple API call would work out of the box. In other words, the plugin itself should use the API the same way a user would. If that's not possible, then I guess the docs should explain how to replicate the builtin mappings using the API methods.

[numToStr]

For instance, .call must be used for operator-pending mode to work, which seems to be a private function

I guess, I could export that as a shorthand for setting operatorfunc :)

and <esc> must called for visual mode.

I can't do much about it. You'll still need to know a little about how VISUAL mode works. Also, we can mention this in docs :)

I think it would be best if these details were abstracted away and a simple API call would work out of the box. In other words, the plugin itself should use the API the same way a user would. If that's not possible, then I guess the docs should explain how to replicate the builtin mappings using the API methods.

Abstracting everything away is not always practical, for example [count]gcc mapping but we can mention how to achieve something in detail in the wiki and docs which I'll tackle next :)

And lastly thanks for trying out the new APIs as soon as I finish the docs I'll merge it.