kylechui / nvim-surround

Add/change/delete surrounding delimiter pairs with ease. Written with :heart: in Lua.
MIT License
3.09k stars 61 forks source link

How to configure after the latest changes? #128

Closed augustocdias closed 2 years ago

augustocdias commented 2 years ago

I'm having issues after the last breaking changes...

Change and delete doesn't work anymore, just add. I suspect I'm configuring it wrong.

return {
    setup = function()
        require('nvim-surround').setup({
            brackets = { '(', '{', '[', '<' },
            surrounds = {
                ['('] = { add = { '(', ')' } },
                [')'] = { add = { '(', ')' } },
                ['{'] = { add = { '{', '}' } },
                ['}'] = { add = { '{', '}' } },
                ['<'] = { add = { '<', '>' } },
                ['>'] = { add = { '<', '>' } },
                ['['] = { add = { '[', ']' } },
                [']'] = { add = { '[', ']' } },
                ['r#"'] = { add = { 'r#"', '"#' } },
            },
            keymaps = {
                insert = false,
                insert_line = false,
                normal = ',a',
                normal_cur = false,
                normal_line = false,
                normal_cur_line = false,
                visual = ',',
                visual_line = false,
                delete = ',d',
                change = ',r',
            },
            highlight = {
                duration = 1000,
            },
        })
    end,
}
kylechui commented 2 years ago

As of right now, the way it's setup is that if you change a surround key from the default, you must provide all other keys, otherwise nvim-surround will default to invalid_key_behavior for the remaining keys. Honestly this should probably be changed to make it simpler for people that just want to change one or two things. For the time being, please copy-paste the surrounds from the default config and change the add keys back to what you would like (in your case, probably only copy the closing pairs).

Also as a quick question: Does your last surround (r#") actually work? I was under the impression that I coded this plugin to work only for single characters.

CC: @andrewferrier @smjonas @akinsho About the configuration question: Should the user have to re-define everything, or just what they want to change? I don't think it would be that hard to change; but also I wonder if it's a better idea to just let the user learn more about the plugin via default configuration :thinking: I'm quite fond of the notion that the plugin itself is very scriptable and exposes some helper functions (e.g. config.get_selection()) to the user to let them do "whatever they want".

More thoughts: Should probably change this since I feel like @augustocdias's configuration is a very common use case; where the user slightly tweaks the defaults and maybe adds new stuff. The current pipeline is:

Proposed pipeline:

For example, in the given configuration where only the add key is given, we first overwrite the default add keys, then check if there are any "missing" keys that we fill in using invalid_key_behavior. Since we are modifying defaults, no such keys need to be filled in, and everything works "as intended". If we instead decided to introduce a new * surround, where only the add key is provided, then it's "merged" into the existing default (which doesn't contain a * surround), and invalid_key_behavior fills in the rest.

Tangent 1: I'm now very glad I decided to rename delimiterssurrounds because it makes these sorts of discussions easier

Tangent 2: More breaking changes :sweat:

akinsho commented 2 years ago

@kylechui I think that whilst script-ability of plugins is great, (from my perspective) the primary use case of nvim is to do some work, scripting the editor is a potential benefit but not it's primary use case, so I personally think plugins should spend more time trying to make sure people can stay productive.

To be honest, in this example I would have just expected that where my change was to a default config options it would just be deeply merged with the defaults so that I wouldn't have to go and redefine a bunch of stuff even if it's just copy pasting.

A bunch of code added to the config like that not only means a user has a more verbose config, but they are also "maintaining" code they might not understand and worse still, code that can break at some point that they never really wanted to have in there anyway.

kylechui commented 2 years ago

That makes a lot of sense; thanks for providing some input. I'll probably work on this soon-ish after I finish Tree-sitter integration (since that seems to be reasonably easy given the way I've refactored some of the code); this change would warrant me releasing v2.0.0 then, right?

kylechui commented 2 years ago

@augustocdias Do you mind trying out the better-configuration branch with the configuration you had earlier and see if that fixes things?

augustocdias commented 2 years ago

Also as a quick question: Does your last surround (r#") actually work? I was under the impression that I coded this plugin to work only for single characters.

This I carried from previous surround plug-in. I forgot to remove when I migrated. I never tried it because the code base I'm working currently doesn't use this pattern.

And I'll test the new branch first thing in the morning

kylechui commented 2 years ago

Yeah just as a forewarning that the surround keys are necessarily one character, so if/when you decide to use that surround you can change r#"r or some other key of your choosing.

andrewferrier commented 2 years ago

As of right now, the way it's setup is that if you change a surround key from the default, you must provide all other keys, otherwise nvim-surround will default to invalid_key_behavior for the remaining keys. Honestly this should probably be changed to make it simpler for people that just want to change one or two things. For the time being, please copy-paste the surrounds from the default config and change the add keys back to what you would like (in your case, probably only copy the closing pairs).

I haven't looked at this specific issue, but this is one of the real cool things about using vim.tbl_deep_extend() to setup your config. For example, look at https://github.com/andrewferrier/debugprint.nvim/blob/e1f932b4bdfe444c0c6fd2d1e9b3a4a4adbcc166/lua/debugprint/init.lua#L105 and https://github.com/andrewferrier/debugprint.nvim/blob/e1f932b4bdfe444c0c6fd2d1e9b3a4a4adbcc166/lua/debugprint/filetypes.lua#L15. If a user passes in a filetypes array to debugprint.setup(), these filetypes are merged into the default filetypes array I already have provided by default automatically, so I just see the merged list. They don't need to pass in the ones that are already in the default list. At least in the case of this plugin, this is the behaviour I want, as that allows them to extend its behaviour and easily benefit from any improvements I make without copy-and-pasting my default config into theirs repeatedly.

Not sure if that helps in your case, but something to bear in mind.

augustocdias commented 2 years ago

@kylechui I've been using your branch and it looks good. Its working as expected

kylechui commented 2 years ago

Ok sounds good, I think I'll have a branch v2.0.0 that I'm going to start merging other branches into.