[Breaking] New interface for adding/deleting surrounds

[kylechui]

One main pain point of this plugin so far is the complete lack of any way to delete custom surrounds, e.g. ds* for de-italicizing text in a Markdown file (see #61). To introduce this feature, it would make sense to let the user specify left/right patterns for nvim-surround to detect, and use those as selections to delete. For example,

-- Setup function call goes here...
["*"] = {
    add = { "*", "*" }, -- Current behavior goes here, since it's additive
    delete = { "\*", "\*" }, -- Some Lua pattern for the left/right to delete it
},
["f"] = {
    add = ..., -- Current implementation of surrounding with function call
    delete = { "%a+(", ")" }, -- Pattern to select the surrounding function call
},

This would greatly enhance the usability of the plugin, but I would like feedback on what the user interface should look like. If anybody has any ideas that don't involve using Lua patterns, I'm also open to hearing about them as well.

Bonus: It seems reasonably common for the user to want to change the surrounding pair without deleting the whole thing:

• With function calls, you only want to change the function name, not the parentheses
• With HTML tags, you only want to change the tag type/inner contents, not the angle brackets

Perhaps an additional key change could be introduced into this table? Somehow this key would let the plugin know which selection to change, and if not present it would default to using the "entire thing" as defined by delete?

[jonatan-branting]

Would it be possible to integrate treesitter to delete/change more complex surrounds such as function calls? I'm thinking it might be possible to leverage the locals queries defined in nvim-treesitter (e.g. https://github.com/nvim-treesitter/nvim-treesitter/blob/master/queries/lua/locals.scm, note the @definition.function definition). In my mind, this would be the most robust solution.

For symmetrical surrounds, a simple table should suffice I reckon.

Regarding the "bonus"-question: I think adding a change key would be very beneficial, as the expectations for delete and change might very well differ in many cases (such as the function call example).

[kylechui]

Tests have been split into several files and completely redone; @smjonas TIL plenary will use your local configuration options if no minimal init is provided, which is why things were failing remotely and passing locally (whitespace issues). All tests are now passing; the only things I have left to do before I think we can release are:

• Allow user input for modifying existing surrounds (see default i key) (won't fix for now)
• Re-read docs to make sure they are good (please help with this!)
• Learn how to tag releases and use SemVer, to let people stabilize their configurations

@andrewferrier I've added two soft deprecation warnings, one when the user still has pairs/separators in their config, and another for when they try to directly assign a key to a delimiter pair, e.g. ["*"] = { "*", "*" }

[kylechui]

Because I intend on using semantic versioning soon:tm:, I'm now somewhat paranoid about naming schemes. I've renamed highlight_motion → highlight because I added support for highlighting the target pair when using cs. However, I'm not sure if I should rename delimiters → surrounds, leaving delimiters to refer to the literal text pairs, and surrounds to refer to the more complex configuration table with add/delete/etc. Since this upcoming version already has quite a few breaking changes, I feel like I should get all my renaming done and out of the way; would much appreciate extra feedback.

For example, I could say:

The `add` key of the surround for `)` adds the delimiter pair `{ "(", ")" }` to the buffer.

I guess this is more of a visual change rather than a functional one, but I'm curious to know if people think this is any better than the current naming.

Also feel free to let me know if I'm just thinking too much about this project 😅, I really want to do this right :)

CC: @andrewferrier @akinsho @jonatan-branting @smjonas @melkster @NoahTheDuke @Gelio

[andrewferrier]

Also feel free to let me know if I'm just thinking too much about this project 😅, I really want to do this right :)

CC: @andrewferrier @akinsho @jonatan-branting @smjonas @melkster @NoahTheDuke @Gelio

I would say you are overthinking it by 20% ;) But honestly, rather that than the opposite... writing stable APIs and configuration is hard. I vote +1 for surrounds personally.

[mawkler]

I also think surrounds is a good name 🙂

[kylechui]

I've merged this into main (#113), and am trying to use semantic versioning (see #120). I didn't want to forget anybody, but massive thanks to everybody in this thread that's helped me out in particular, you guys have really helped make nvim-surround what it is today!

[kylechui]

Since I can't be bothered to open up a whole new issue, I'm using this thread for declaring (un)intentional breaking changes, as well as a place for you guys that are testing the "beta" to report any bugs. I'll be continuously updating this top-level comment as new issues get found, and old issues get resolved. If there are any intentional changes on this list that you object to, I'm open to discussion in the replies.

Intentional Changes

• pairs, separators, and delimiters have been deprecated in favor of storing everything directly in surrounds
• highlight_motion has been renamed to highlight
• Configuration for surround keys has been redone and now contains the four (five?) keys add, delete, find, change.target, and change.replacement
• Passing an argument table into the add key's function is no longer available, e.g. easy access for the selection's contents is not supported
• The behavior for modifying deleting/changing open pairs is slightly different, see this commit message for more info
• The default invalid_key_behavior is now to add/modify the raw characters, instead of throwing an error

Unintentional Changes

• Modifying functions errors out because one of their selections is nil
• If there are an odd number of quotes on a line, then modification fails on the final quote (e.g. "hello world " more quotes"); unsure if this will be fixed
• Modifying surrounds is inconsistent for repeated characters; ds* for the following:

*hello*
*world*

Fixed by 3d8c197

• Surrounding empty lines errors out (ys, yss, S, but not VS?) [Fixed by af61634]
• Jagged visual-block surrounds error out (if the block goes out of bounds for any of the text lines) [Fixed by af61634]
• There is no way to delete or change (target) based on user input (won't fix for now as it requires non-trivial refactors; I'll consider it in the future if people ask for it)
• Insert-mode surrounds are one character off at the end of a line [Fixed by 6e5d744]

[smjonas]

I am on the latest commit on the pattern-matching branch and the errors occur even in an empty buffer when typing dsq / css / dss.

[kylechui]

@smjonas Honestly I'm not entirely sure why targets.vim causes this issue, since looking at the code it almost seems to be solely a problem on my end 🤔 In any case, it should be fixed in the latest update.

Edit: Unsure if jumping from L1 to L2 for your minimal test case is intended behavior introduced by targets.vim though, as I've never really used it extensively

Edit 2: Seems like this is the case, as per target.vim's documentation:

Quote text objects work over multiple lines and support |targets-quote-seek|.

Perhaps they use a different heuristic than base Vim for setting marks when the text-object doesn't exist.

[smjonas]

Thank you, it's fixed now! One question, not sure if this behavior is intended:

• using csqb changes "" to (), but csqq changes "" to qq?

[kylechui]

Yes that is intended behavior:

• csq targets the nearest pair of quotes, then since the change.replacement key isn't set for " (which is the pair that's found), the user is queried for a replacement char
• User inputs q, which is not in the surrounds table, so nvim-surround falls back on invalid_key_behavior's add key
• The default is to duplicate and surround, which is what happens

I can't think of a reasonable implementation for what the second q should do otherwise, if it is only defined as a tabular alias. Someone mentioned in #98 that they might want to see it used to toggle quotes, but that seems like extra effort on my end that I don't really want to get into at the moment.

[smjonas]

That makes sense, I also just noticed that q is ambiguous because it can mean either single quotes or double quotes (which is not the case for b which always means ( / )). Thanks for clarifying that!

[kylechui]

I think it's probably time to start drafting up a proper breaking changes announcement, then? 👀