Improve API design

[logan-connolly]

As mentioned in #7, I think there is an opportunity to improve the API of this package. Namely, shifting the focus around the actions as opposed to the symbols. From my point of view, this would have two main benefits:

1. Simplify the number of commands; currently the number of commands is a function of n_actions * n_icon_sets;
2. Combine the character sets as you see fit, e.g. Emoji + Nerd.

I think the easiest way to accomplish this is by making it into a lua package that exposes actions. Here is a very rough mock up of what the API could look like:

-- Option 1: one method option
require('icon-picker').select({ mode = "yank", icons = {"emoji", "nerd"} })
require('icon-picker').select()  -- default to mode='paste' and icons='everything'

-- Option 2: two method option
require('icon-picker').paste({ icons = {"emoji", "nerd"} })   -- combine icon sets
require('icon-picker').yank()  -- default to icons='everything'

You may have noticed that I did not add insert mode as an option. The reason is that my hunch is that it is more of a matter of how you set the keybinding, as opposed to the actual function that is being used. Here is an example of how I imagine the key map could be set:

vim.keymap.set({'n', 'i'}, "<c-i>", function()
    -- paste the selection in both 'normal' and 'insert' modes when ctrl + i is pressed
    require('icon-picker').select({ mode = "paste", icons = {"emoji", "nerd"} })
end)

[ziontee113]

Thank you for opening the issue and your insights on improving the API design. I agree that the number of commands right now is not ideal.

I quite like your 2nd option (two method option). It leads me to think that we can consider having arguments in our Vim commands. Something like: PickIcons nerd emoji alt.

I didn't take the amount of commands into account when making the API. Changing the API now is not the difficult part, but how can we support users who're still using the old style commands.

Not having command arguments was an oversight and a mistake. I suppose we could create a new branch that uses the new API for more "cleanliness", but is it necessary?

Again, thank you so much for the PR and the issue. I'd love to hear your input on this. Cheers 👍

[logan-connolly]

Hey @ziontee113, thanks for getting back to me so quickly!

I quite like your 2nd option (two method option). It leads me to think that we can consider having arguments in our Vim commands. Something like: PickIcons nerd emoji alt.

Yeah definitely, so both options would support adding an icons parameter with type table (list) of icon groups to include, e.g. icons = {'nerd', 'emoji', 'alt'}. Perhaps my initial comment was not clear on that point.

I didn't take the amount of commands into account when making the API. Changing the API now is not the difficult part, but how can we support users who're still using the old style commands.

I like where your head is at! We definitely do not want to be breaking everyone's config overnight :) I think there are three options (please expand this list if you think of another option):

1. New branch strategy: not ideal because you split development into two branches. You need to consistently make sure that each branch gets updates applicable to both; It just increases maintenance and puts the burden on the user to configure their respective packaging tool (packer, Plug, etc.) to select the right branch.
2. Hard fork: the project could be forked to pursue the new architecture, while ziontee113/icon-picker.nvim's API remains stable for active users. I think this is not ideal because this is your creation and it would be great to improve on it as opposed to splitting off it. I also think it confuses people in the neovim community when there are two packages that essentially do the same thing.
3. Deprecation (recommended): basically adding support for the lua commands in parallel while not changing the existing vim commands. Once the lua commands are ready, update the documentation with the new commands and add a line in the legacy commands saying that the old commands will be phased out:

local function insert_user_choice_normal(choice)
  vim.notify("WARNING: this command will be deprecated on YYYY-MM-DD with version 1.0.0. Please use new lua command `require('icon-picker').select`. See README for more information.")
  if choice then
    local split = vim.split(choice, " ")

    vim.schedule(function()
      vim.cmd("norm! a" .. split[1] .. "�")
    end)
  end
end

You can add a specific date if you like and/or the version number when it will be deprecated (currently there are no versions, but you could consider add them). Here is a good resource on semantic versioning if you decide to version this project: https://semver.org/

I am looking forward to hearing from you! 😃 <- emoji yanked from icon-picker.nvim

[ziontee113]

Deprecation (recommended)

Thank you for the suggestions. I think I've found the 4th solution, which is to give the user the choice to which type of commands (lua or vim cmd) to use.

Since users will have to require("icon-picker") anyway, having a setup() function would be ideal imo. It can look something like:

require("icon-picker").setup({
    command_type = "arguments" | "legacy" -- legacy by default, for backwards compatibility
})

So we & the user can have the flexibility of choosing commands without breaking any existing users' config.

legacy is like the current version.

arguments means something like PickIcons nerd emoji alt.

And the user will always have the option to use vim.keymap.set on lua functions.

require('icon-picker').select({ mode = "paste", icons = {"emoji", "nerd"} })

[ziontee113]

Hi @logan-connolly , I've implemented the "arguments" method. With that approach I don't think users will need a Lua API for their mappings, since they can cherry pick the sources with arguments. 👍

[logan-connolly]

Since users will have to require("icon-picker") anyway, having a setup() function would be ideal imo. It can look something like:

I agree that having a setup function is good to have, but I think it does not prevent "breaking" people's configs. When I updated your package today, I got an error because I wasn't calling setup.

I appreciate you taking the time to work through the comments though. I wish you the best in developing this plugin! and most importantly keep it fun :)

[ziontee113]

I agree that having a setup function is good to have, but I think it does not prevent "breaking" people's configs. When I updated your package today, I got an error because I wasn't calling setup.

Thank you for your feedback. I've added a fix so if users don't call the setup function, they will get both the old commands and the new ones.