supermaven-inc / supermaven-nvim

The official Neovim plugin for Supermaven
https://supermaven.com/
MIT License
279 stars 16 forks source link

feat: allowing for `config = true` or `require(...).setup()` #22

Closed AlejandroSuero closed 1 month ago

AlejandroSuero commented 1 month ago

Changes description

With these changes it will allow for more user options when calling setup.

https://github.com/supermaven-inc/supermaven-nvim/blob/98c2821c985ec751df46c033c5494079dce61522/lua/supermaven-nvim/config.lua#L12-L15

With the changes, now it forces an empty table into the default_config into the args or an empty table if args are nil.

Summary

Test results ```txt Running vusted tests ok 1 - setup should set up the plugin's user commands ok 2 - setup should set up the plugin with the default config ok 3 - setup should set up the plugin with a custom config 1..3 # Success: 3 Running tests using nvim Starting... Scheduling: ./tests/supermaven-nvim/setup_spec.lua ======================================== Testing: /Users/aome/dev/nvim_plugins/supermaven-nvim/tests/supermaven-nvim/setup_spec.lua Success || setup should set up the plugin's user commands Success || setup should set up the plugin with the default config Success || setup should set up the plugin with a custom config Success: 3 Failed : 0 Errors : 0 ======================================== ```

The test suite could be added to Github workflows if needed. Example of test suite workflow on freeze-code.nvim.

sm-victorw commented 1 month ago

Looks great. Only issue I'm noticing is that "Supermaven" is spelt "SuperMaven" throughout the config and init files. Apologies for not having a proper contribution/style guide

The review may take a bit longer than usual, since I'm not familiar with the vusted framework

AlejandroSuero commented 1 month ago

@victorw-xyz I can fix those in the meanwhile if you want me to. I was using supermaven to populate the classes and I missed that fact sorry.

jakevollkommer commented 1 month ago

looking forward to trying the plugin once this gets merged

AlejandroSuero commented 1 month ago

The review may take a bit longer than usual, since I'm not familiar with the vusted framework

@victorw-xyz, from what I know about vusted is that is a wrapper around busted, so it is acting similar to plenary.nvim when using :PlenaryBusted{File|Directory.

With busted it self you have to configure it to work per project, vusted and plenary.nvim are knowledgable of the files used in the require(...) from your project.

[!NOTE] Resources on how to configure.

From rocks.nvim

From a neovim discussion

With all that, at least when I configure it using MacOS 14.4.1, I get the following error:

busted: error: Cannot load output library: utfTerminal

So I usually stick to vusted and plenary.nvim for testing neovim plugins, busted seems to work fine for games and other types of embedded applications I feel like.

I used vusted for the first time contributing to nvim-lspconfig

sm-victorw commented 1 month ago

My understanding of vim.tbl_deep_extend is that when the second table is nil, the function errors out and halts execution. I don't see why this would result in #19, and I haven't been able to replicate the json related error that #19 was reporting.

When testing this myself by deliberately passing in a nil table for the value of args, the download completes fine, which I believe is the expected behavior since the first line of init.lua loads and runs binary_handler (where the download occurs) and this occurs before the setup function is called.

Are you able to give steps that reproduce #19?

AlejandroSuero commented 1 month ago

@victorw-xyz I was trying to reproduce it and I couldn't, It happened to me the first time I loaded the plugin, but I couldn't get to reproduce it again.

When I made the changes locally and got it working I thought that was what it fixed it.

To test it the first time I used this config after rm -rf ~/.local/share/nvim:

-- ~/.config/nvim/init.lua or in a minimal.lua and run:
-- nvim --clean -u minimal.lua
local lazypath = "/tmp/lazy/lazy.nvim"
if not (vim.uv or vim.loop).fs_stat(lazypath) then
  vim.fn.system({
    "git",
    "clone",
    "--filter=blob:none",
    "https://github.com/folke/lazy.nvim.git",
    "--branch=stable", -- latest stable release
    lazypath,
  })
end
vim.opt.rtp:prepend(lazypath)
require("lazy").setup({
    {
      "supermaven-inc/supermaven-nvim",
      config = function()
          require("supermaven-nvim").setup({})
      end,
    },
}, {})

And when using it with the default way I load plugins require("supermaven-nvim").setup() it failed in config with this stacktrace (different from #19):

Failed to run `config` for supermaven-nvim

vim/shared.lua:0: after the second argument: expected table, got nil

# stacktrace:
  - vim/shared.lua:0 _in_ **validate**
  - vim/shared.lua:0 _in_ **tbl_deep_extend**
  - /supermaven-nvim/lua/supermaven-nvim/config.lua:15 _in_ **setup_config**
  - /supermaven-nvim/lua/supermaven-nvim/init.lua:10 _in_ **setup**
  - ~/.config/nvim/init.lua:18 _in_ **config**
  - ~/.config/nvim/init.lua:13

https://github.com/supermaven-inc/supermaven-nvim/blob/264768c6b2a2e0480868e9dae443112e33b1484a/lua/supermaven-nvim/config.lua#L14-L17

config = vim.tbl_deep_extend("force", config, args or {}) -- this allows to work as well
config = vim.tbl_deep_extend("force", {}, config, args or {}) -- this is the default way I've see it being done through almost all plugins (telescope.nvim, plenary.nvim, etc.)
config = vim.tbl_deep_extend("keep", args or {}, default_config) -- this also works and I think it could be the best of the three if you want me to change it.

I have the last option committed in my fork in a branch made after from main after 264768c, in wich I was trying to trigger the issue #19.

You can test it with:

-- ~/.config/nvim/init.lua or in a minimal.lua and run:
-- nvim --clean -u minimal.lua
local lazypath = "/tmp/lazy/lazy.nvim"
if not (vim.uv or vim.loop).fs_stat(lazypath) then
  vim.fn.system({
    "git",
    "clone",
    "--filter=blob:none",
    "https://github.com/folke/lazy.nvim.git",
    "--branch=stable", -- latest stable release
    lazypath,
  })
end
vim.opt.rtp:prepend(lazypath)
require("lazy").setup({
    {
      "AlejandroSuero/supermaven-nvim",
      branch = "testing/macos-14-4-1-json",
      config = function()
          require("supermaven-nvim").setup({
             keymaps = {
                accept_suggestion = "<C-k>",
             },
             color = {
                suggestion_color = "#DC8CE2", -- blueish color
                cterm = 117,
             },
          })
      end,
    },
}, {})
sm-victorw commented 1 month ago

19 seems to be addressed by #34.

Regarding the testing framework, I think we should avoid introducing external frameworks whenever possible. It may be something we will want to revisit if the repository becomes large and significantly more complicated.