WIP Integrations API Proposal (+ whichkey and mapper integrations)

[connorgmeehan]

Hi LionC, this is an preliminary implementation and proposal for an integrations API. Right now this works but I'm sure it has a lot of bugs that need to be ironed out and plenty of room for improvement. I'm looking for feedback on this architecture to make sure that I'm heading in the right direction. I want to make sure that we land on a solid, easy to use API on release so everything is open to criticism and feedback :)

Proposed API

nest.enable(require('nest.integrations.mapper'))
nest.enable(require('nest.integrations.whichkey'))

nest.applyKeymaps { '<leader>', {
        { 'm', '<cmd>Telescope mapper<cr>', 'Mapper'},
        { 'p', require'telescope.builtin'.planets, 'Planets' },
        -- Prefix every nested keymap with f (meaning actually <leader>f here)
        { 'f', name = '+File', {
            { 'f', '<cmd>Telescope find_files<cr>', 'Find Files' },
            -- This will actually map <leader>fl
            { 'l', '<cmd>Telescope live_grep<cr>', 'Search Files', },
            -- Prefix every nested keymap with g (meaning actually <leader>fg here)
            { 'g', name = '+Git', {
                { 'b', '<cmd>Telescope git_branches<cr>', 'Branches' },
                -- This will actually map <leader>fgc
                { 'c', '<cmd>Telescope git_commits<cr>', 'Commits' },
                { 's', '<cmd>Telescope git_status<cr>', 'Status' },
            }},
        }},

Integration API

-- This is an example integration to help future users implement their own nest.nvim integration
-- This integration will log your nest config for you by pretty printing your applyKeymaps config
--
--- @type NestIntegration
local module = {}
module.name = 'example';

-- Utility length function
local len = function(table)
  local length = 0;
  for _, _ in ipairs(table) do
    length = length +1
  end
  return length
end


--- This is run before the keymaps are bound
--- Use it to setup data structures before the applyKeymaps config is traversed
--- @param config table<number, NestNode>
module.on_init = function(config)
  print('nest.nvim starting keymap binding');
end

--- This will be called for every element in the applyKeymaps config.
--- Here you can pass the nest.nvim config to whatever library you're integrating with 
--- or build a different representation of the data to be applied in the on_complete function.
--- @param node NestIntegrationNode
--- @param node_settings NestSettings
module.handler = function (node, node_settings)

  -- If node.rhs is a table, this is a group of keymaps, if it is a string then it is a keymap
  local is_keymap_group = type(node.rhs) == 'table'

  if is_keymap_group then
    print('nest.nvim: ' .. node.lhs .. ' is group of ' .. len(node.rhs) .. ' keymaps.')
  else
    print('nest.nvim: ' .. node.lhs .. ' is keymap binding to ' .. node.rhs .. '.')
  end

  -- You can get different keymap settings from the node_settings object
  local msg = '    buffer: ' .. string.format('%s', node_settings.buffer) .. ' | mode: ' .. node_settings.mode .. ' | '
  if (node_settings.options.noremap) then
    msg = msg .. 'noremap,'
  end
  if node_settings.options.silent then
    msg = msg .. 'silent,'
  end
  if node_settings.options.expr then
    msg = msg .. 'expr'
  end
  print(msg)
end

--- This is run before the keymaps are bound
--- Use it cleanup or apply data that was created in the handler/on_init functions
module.on_complete = function()
  print('nest.nvim: finished keymap binding.');
end

return module;

Change list:

• Allows specifying a name for a keymap using either the third element in the table or the name property
• Allows specifying a description for a keymap using either the fourth element in the table or the description property.
• Added enable function to nest module allowing users to pass in integrations with different plugins.
• Split applyKeymaps into applyKeymaps and traverse (generic function for traversing over they keymap config)
  • Needed to facilitate the on_init/on_complete functions on integrations
  • Also handy having traverse seperate as it allows the user to run or avoid running a specific integration if they need to for a one off.
• Added a (very basic and untested) whichkey integration
• Added a (very basic and untested) nvim-mapper integration

Points of concern

• I will probably do another pass on the names for the api when I've had more sleep, but if you have any feedback that'd be much appreciated.
• I can remove the Makefile etc for release, is just helpful for development.

How to test

• Checkout my fork in a folder that also has plenary.nvim, whichkey, telescope and nvim mapper in it.
• Cd into nest.nvim
• Run make test

[LionC]

Hey @connorgmeehan

thank you for the thorough PR! Just a heads up: I will probably not have time until the weekend to dive deep into it. I just wanted to take some seconds to acknowledge it and let you know that I will take a look at it.

[connorgmeehan]

Hey @LionC, any chance you can check this out on the weekend?

[LionC]

Looking at it now - sorry for the delays, might take a bit longer in total

[connorgmeehan]

Hey again @LionC, no worries! It's a very busy time of year.

I've done some refactoring on top of my previous changes, and over-all I'm pretty happy with everything now. I've tested the integrations and it looks like everything is working ok including buffer keybindings which was my main point of concern. I've also improved the architecture.

I'll describe the changes of the whole PR to help the review process:

• Added "integrations"
  • An integration takes the nest.nvim API and does something.
  • Has a on_init (before traversing nest config) on_complete (after traversing nest config) and handler (handles a keymap or keymap group) function.
  • Check lua/nest/integrations/example.lua for an example
• Added an enable function that allows user to enable an integration.
  • Example: nest.enable(require('nest.integrations.example'))
  • This must be run before applyKeymaps to take effect.
• Moved the default behaviour of binding keymaps into its own "nest" integration that is automatically enabled.
• Split applyKeymaps into two functions
  • applyKeymaps runs the on_init of all integrations, then traverses the config, then runs on_complete of all integrations.
  • traverse navigates over all nodes of the nest config and passes the data to the integrations. Does not bind key maps, relies on the "nest" integration to do that
• Removed duplicated code
  • copy and merge functions are duplicates of existing nvim api.
• Documented integrations in including example config README.md

Also it'd be really cool to have debug integration that performs more checks on the provided nest config to catch any errors, like right now I don't think nest.nvim can handle nils in the rhs (occurs when referencing a lua function that doesn't exist).

Let me know what you think! 😄

[LionC]

First of all: thank you a lot for the amount of work and thought put into this. I really appreciate the architectural ideas and the written explanations to go with it.

I think I like the overall approach, it goes in a very similar direction to where I was thinking but a lot more thought out and actually done^^ I think integrations / modules / whatever will need to be able to add public APIs as well, that is at least how I started drafting a solution for #24 and I think it fits this design well enough.

It will take some more days for me to find the time to review this line by line. The only concern I could maybe see with that architecture is that the core "key binds only" functionality (which will stay the most popular use) is affected by this performance wise, but if that is insignificant, it should be fine.

[alaindresse]

I'm trying the whichkey integration, and I really like it !
Thank you !

[Pranav-Badrinathan]

Great work! It is really well done, and I could not find any obvious breaking bugs in the code itself. However, I wanted to confirm exactly how the nvim-mapper integration works:

So basically, every set of { } with an rhs that is another table and a non-nil name is a category?

{ 't', name = 'telescope', {
    { 'f', name = 'find', {
        {'f', 'Telescope find_files'},
        {'g', 'Telescope live_grep'}
    }},
    { 'a', 'foo' },
    { 'b', 'bar' }
}}

would have a category structure of:

>Telescope
|\__> find
      |  find_files
      |  live_grep
|  foo
|  bar

and removing the name find makes it

>Telescope
|  find_files
|  live_grep
|  foo
|  bar

right?
Amazing work, thanks for this! I was going to do this myself, but then I discovered this PR and I have to say, you've done better than I could even think of!

P.S: This is my very first review. If my comments are unnecessary, or do not follow an unspoken 'code of conduct', do let me know!

[Pranav-Badrinathan]

Here the example uses <leader>f for Telescope find_files, and in other spots (line 264 and original README) it uses <leader>ff
Suggestion: change it all to <leader>ff, just for consistency.

[Pranav-Badrinathan]

default_integration could be pulled out into it's own file, for consistency, because the other integrations are in their own files anyways.
Only reason to not would be if some user accidently nuked their integrations folder, having this in nest.lua would enable them to still run the barebones version of the plugin.
If someone deletes integrations, they can re-clone the rep. So my vote is on extract for consistency.

[connorgmeehan]

I agree with splitting it into its own file but one of the selling points in the README.md was that it was a single lua file. Interested what @LionC thinks. On the other hand given that the vim.keymap api is merged it could be worth having two default integrations, one for the new API (no need to manage a table of lua functions) and one for the old API. Not sure if there's any benefit to doing this however.

Also for your last point, I don't think users will ever be deleting the contents of the integrations folder, it's just supposed internal to nest.nvim. If a user want's to add their own custom integration they can just create an object with the same fields as the example integration and pass it using nest.enable().

[Pranav-Badrinathan]

...one of the selling points in the README.md was that it was a single lua file.

I completely missed that! This PR would also crush the 100 lines claim, though. Maybe 2 separate branches could serve different versions, one that's minimal and the other integrated. Though that certainly raises up the ante for maintaining this project quite a bit.

On the other hand given that the vim.keymap api is merged it could be worth having two default integrations, one for the new API (no need to manage a table of lua functions) and one for the old API. Not sure if there's any benefit to doing this however.

Oh wow! I had no idea they were doing this. It certainly would help pushing all the lifting onto the Nvim API itself.(also reduce lines of code, if that philosophy is still maintained).
And no, I don't think there would be a benefit maintaining for an older API. I mean, we could merge this PR as is and then modify it for new API, redirecting all who want the old API to the merge commit 🤷. Or if it is that troublesome, just have the old stuff be fallback. Adds bloat, but backwards compatibility.

OR (Ikr, too many ideas!) just split breaking stuff into different file with wrappers around the breaking stuff, and ask users to include whichever one fits their Nvim version.

Also for your last point, I don't think users will ever be deleting the contents of the integrations folder,...

Yeah, I know, I was just thinking of worst case scenarios. Just a couple of days ago packer kinda broke on me and would not delete the plugin folder when removing it, only it's contents. That led to it asking me repeatedly if I wanted to remove it, and not doing so, rinse and repeat. I got fed up, went to the plugins folder and did stuff that broke packer! So yeah.

[connorgmeehan]

Hey, thanks for reviewing the PR. I really appreciate it.

Yep that's right, I'll annotate some examples with the categories.

{ 't', name = 'telescope', {
    { 'f', name = 'find', {
        {'f', 'Telescope find_files'}, -- (category = find)
        {'g', 'Telescope live_grep'} -- (category = find)
    }},
    { 'a', 'foo' }, -- (category = telescope)
    { 'b', 'bar' } -- (category = telescope)
}}

However, right now if the name for the old find keymap group is nil the category will be unknown.

{ 't', name = 'telescope', {
    { 'f', {
        {'f', 'Telescope find_files'}, -- (category = unknown)
        {'g', 'Telescope live_grep'} -- (category = unknown)
    }},
    { 'a', 'foo' }, -- (category = telescope)
    { 'b', 'bar' } -- (category = telescope)
}}

Maybe this is something I should fix using the same merging/overwriting behaviour as the keymap settings (noremap/silent etc).

[Pranav-Badrinathan]

Yeah, something I completely forgot to mention in the review was that if you set or 'unknown' here, your nil check a few lines below is basically pointless as it is never going to be nil.

And I do prefer the Unnamed ones falling into the closest top category, not a new one as that beats the entire idea of nest.nvim, as it does not respect the hierarchical structure setup by the user 😅.

Also, would other, totally unrelated mappings also fall under this? That would make no sense! 😫
Eg:

{
    { 't', name = 'telescope', {
        { 'f', {
            {'f', 'Telescope find_files'},
            {'g', 'Telescope live_grep'}
        }},
        { 'a', 'foo' },
        { 'b', 'bar' }
    }},
    { 'a', name = '42istheanswer', {
        { 'r', {
            {'s', '42!'},
        }},
        { 'a', 'foo' },
        { 'b', 'bar' }
    }}
}

Would r in 42istheanswer fall in the same group as f in Telescope?

EDIT: Formatting

[connorgmeehan]

Yeah I think you're right, it makes more sense for these fields to propogate like like the rest of nest. There are some fields that you probably wouldn't want to bubble such as the name field and the uid for mapper but I think having a single behaviour is easiest for the user to understand and it means we can simplify some of the logic in the mapper integration.

Also to answer your question no, the config is traversed by depth first so any fields set will only propagate to child keybindings. If they're siblings there shouldn't be any cross over.

I'll work on implementing this soon.

[Pranav-Badrinathan]

...easiest for the user to understand and it means we can simplify some of the logic in the mapper integration.

Always a good sign when the UX is improved AND the code is simplified. 😁

Good luck with this, and thank you!