Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add commands from nvim_get_commands #425

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ibash
Copy link

@ibash ibash commented Jan 29, 2024

This adds commands from nvim_get_commands and attempts to filter out non-human-friendly descriptions.

Contributes #259

How to Test

  1. Please leave detailed testing instructions

  2. Enable built in commands

  3. Run :Legendary

  4. Look for added commands and check to make sure that non-human-friendly descriptions are not present.

Testing for Regressions

I have tested the following:

  • Triggering keymaps from legendary.nvim in all modes (normal, insert, visual)
  • Creating keymaps via legendary.nvim, then triggering via the keymap in all modes (normal, insert, visual)
  • Triggering commands from legendary.nvim in all modes (normal, insert, visual)
  • Creating commands via legendary.nvim, then running the command manually from the command line
  • augroup/autocmds created through legendary.nvim work correctly

This adds commands from nvim_get_commands and attempts to filter out non-human-friendly descriptions.

Contributes mrjones2014#259
@ibash ibash changed the title Add commands from nvim_get_commands and nvim_buf_get_commands Add commands from nvim_get_commands Jan 29, 2024
Copy link
Owner

@mrjones2014 mrjones2014 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey, thanks for the PR! Please make sure to read CONTRIBUTING.md.

A couple of additional thoughts; with this approach, we'll likely get duplicated items, e.g. if you're using any extensions to load commands from other plugins, this approach would load them again. It will also load all the commands that are already specified in the static builtins tables.

Also what happens if new commands are registered later after startup? This approach won't load those.

If you're fine with those tradeoffs, then this functionality would be perfectly suited to be implemented as an extension (in a separate repository), but I don't think this approach is well suited to be built into the core plugin.

However, I do think there's room to improve the approach here (maybe we can monkey-patch nvim_create_user_command or something?). I'll leave the PR open and see if anyone else from the community has comments.

Comment on lines +26 to +32
return string.len(description) > 0
and not string.find(description, "^exe ")
and not string.find(description, "^:exe ")
and not string.find(description, "^call ")
and not string.find(description, "^:call ")
and not string.find(description, "^echoerr ")
and not string.find(description, "^lua require")
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pattern matching this many times may have performance issues. We can use this tool to test Lua patterns, we should use a single pattern match that would match any of these.

@@ -1,5 +1,37 @@
local M = {}
-- commented out ones need a way to get input after selecting

-- get global user-commands
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Additionally, these aren't necessarily built in unless you pass { builtin = false }, so I don't think builtins.lua is the right place for this.

Comment on lines +69 to +71
State.items:add(vim.tbl_map(function(keymap)
return Command:parse(keymap, true)
end, Builtins.get_commands()))
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doing this here also means we only get commands during startup, not commands that might be registered later, e.g. by lazy-loaded plugins.

Comment on lines +12 to +13
-- description cannot be the empty string or the command doesn't show
description = " "
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is by design; we shouldn't be using a blank space to circumvent our own rules.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants