plugins/utils/nvim-osc52: add desc option #1704
Conversation
Also adds periods to the ends of the option descriptions.
PowerUser64
force-pushed
the
osc52-keymap-desc
branch
from
29753e9
to
a3441d0
Compare
There was a problem hiding this comment.
I think this PR highlights another plugin that needs converting to mkNeovimPlugin and some keymap options that ought to be standardized.
It also looks like we're currently setting keymaps for this plugin by default, which goes against our general principle of minimal magic and explicit opt-in configuration.
That said, this PRs changes have a low maintenance cost. Unless someone is happy to work on refactoring the plugin, I think we can go ahead with this PR?
I left some general thoughts and comments below. Apologies for rambling.
| type = types.str; | ||
| description = "Keymap description."; | ||
| default = "Copy using OSC52"; | ||
| }; |
There was a problem hiding this comment.
I think a better approach here would be to refactor the keymap's option in line with #603 (i.e. using mkMapOptionSubmodule)
If that helper doesn't currently suit this use-case we should consider either making the helper more flexible or changing how the keymaps option here works (breaking changes if absolutely necessary).
That said, this is another plugin that needs a switch to mkNeovimPlugin, so IMO it isn't worth refactoring anything until we make that larger switch.
| @@ -65,6 +71,7 @@ with lib; | |||
| action.__raw = "require('osc52').copy_operator"; | |||
| options = { | |||
| expr = true; | |||
| desc = "Copy using OSC52"; | |||
There was a problem hiding this comment.
Shouldn't this also inherit desc from cfg.keymaps like the others?
| @@ -74,14 +81,18 @@ with lib; | |||
| action = "${copy}_"; | |||
| options = { | |||
| remap = true; | |||
| inherit desc; | |||
There was a problem hiding this comment.
Alternatively, instead of adding a desc option here, we could use the options.plugins.nvim-osc52.keymaps.*.description value?
Maybe that's a little over-engineered...
Do you think it's ok that all keymaps have the same desc though? Maybe desc could use:
{
# allow functions or strings
type = either str (functionTo str);
# normalise string to a function that ignores its argument
apply = v: if isStr v then x: v else v;
# Sane default uses the context provided
default = name:
"Copy "
+ (optionalString (name != "") "${name} ")
+ "using OSC52.";
}The option would then be a function (str -> str) that maps ["" "visual selection" "line"] to a keymap desc.
The keymaps had no description, so I added this option to set it. Also, I added periods to the ends of the option descriptions.