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

Release v2.0.0 #136

Closed kylechui closed 2 years ago

kylechui commented 2 years ago

Breaking Changes

TODO: In the actual release notes, just link to #77

Some user configuration variables have been renamed to better reflect their actual purpose:

-- Old configuration
require("nvim-surround.config").get_selection({
    textobject = ")",
})
-- New configuration
require("nvim-surround.config").get_selection({
    motion = "a)",
})

New Features

Bug Fixes

Future Goals

kylechui commented 2 years ago

Hmm I didn't realize that LSP would actually tell you about that; the reason why I initially omitted them is because when nvim-surround stores everything in a buffer-local variable, it actually "translates" all user configuration to a standardized, internal format. For example add = { "(", ")" } gets converted to add = function() return { { "(" }, { ")" } } end. For the time being this will work just fine, but I wonder if I should create two sets of type annotations, one for user-facing configuration and one for the internal workings. Thanks for the comment @latipun7

smjonas commented 2 years ago

One suggestion for this release: For custom surrounds, it is often the case that delete and change.target need to be set to the same pattern. I would even say that this is the case for the majority of surrounds. Examples: simple character surrounds such as surrounding with ** or a function call such as pretty_print in Lua.

Would you consider falling back to the delete pattern if change.target is nil? This would reduce code duplication for the user and simplifies the API. It would still be possible to disable change entirely if set to an empty function, right?

Example: Old:

    p = {
      add = { "vim.pretty_print(", ")" },
      find = "vim%.pretty_print%b()",
      delete = "^(vim%.pretty_print%()().-(%))()$",
      change = {
        target = "^(vim%.pretty_print%()().-(%))()$",
      },
    },

New:

    p = {
      add = { "vim.pretty_print(", ")" },
      find = "vim%.pretty_print%b()",
      delete = "^(vim%.pretty_print%()().-(%))()$",
    },

csp will still work as before.

Let me know what you think!

kylechui commented 2 years ago

@smjonas Very good point. I suppose change is only different from a literal add/delete pair if it's more complex (e.g. function call, HTML tag), and isn't actually necessary most of the time. I'll add this in at some point; still working on user interface for default filetype-specific surrounds. Design is hard :sweat_smile:

kylechui commented 2 years ago

@smjonas Added in the latest commit, thanks for the great suggestion!

smjonas commented 2 years ago

Awesome, thank you for implementing that so fast!

kylechui commented 2 years ago

If you actually look at the commit it was honestly a really quick fix, just added like 3-4 lines :)

kylechui commented 2 years ago

Oh wait @smjonas an interesting (new) issue cropped up as a result of defaulting to delete; in the case that a buffer-local setup does not define change.target, should it default to that surround's delete key, or the global surround's change.target key? In particular, if the f surround for Lua files is set by

require("nvim-surround").buffer_setup({
    surrounds = {
        ["f"] = {
            find = function()
                return config.get_selection({ node = "function_call" })
            end,
            delete = "^([^=%s]+%()().-(%))()$",
        },
    },
})

then csf will include the closing parenthesis, since it is falling back on the delete key. I'm not sure if changing it to look globally first will cause any other unforeseen issues though :/

smjonas commented 2 years ago

I would suggest to only fallback to the local surround's delete key. This way, users can immediately tell what will happen without having to look at the global configuration first. My suggestion was mainly intended to reduce duplicate code within a single surround.

kylechui commented 2 years ago

Hmm... this behavior might actually be unrelated; will do some more investigating.

Edit: It was unrelated. Oops!

kylechui commented 2 years ago

@smjonas I've decided on falling back on globals first, since I don't think it makes sense that tweaking delete for function calls for the current filetype should affect tweaking change.target for the same filetype. The omission of the change key is just syntactic sugar, but they should remain separate I feel.

latipun7 commented 2 years ago

Great 🎉 Waiting for this PR appeared in a release tag 👍 (since I install this plugin pinned to latest tag 😃)

kylechui commented 2 years ago

@latipun7 There are (unfortunately) still quite a few bugs that need to be ironed out, as well as user config interface decisions that need to be made. This most likely will not appear in a release for a little bit, unless something magical happens and I'm satisfied with the quality of the code.