add optional surround package#20596
Conversation
Initially I wanted to have the same order as surround.vim, however, the implementation is then:
Which I wanted to avoid. So it is PS, it should be easy with |
|
Let me try one additional idea, maybe it would work. |
I knew, I was missing something really simple to actually make it A simple boolean var solved the issue I was trying to tackle for many other hours. |
|
can we have https://github.com/tpope/vim-surround/blob/master/doc/surround.txt#L138 |
|
Hm, thanks but this is quite a bit of code you are adding here. What do others think? |
we can, although it would be a bit more code :) |
|
@chrisbra, it is indeed more code than some of the other optional packages, but it is less than helptoc, matchit or editorconfig. Not to mention netrw. And surround ops themselves are quite useful. I would prefer them to be implemented in the core, but I believe optional plugin is way less intrusive. |
except for surround_v_S
|
This is done. @bennyyip I will add function surround add/remove/change later. |
touché :) |
There was a problem hiding this comment.
Pull request overview
Adds a new optional Vim package (surround) that provides surround/add/remove/change operators and ships runtime documentation plus a new test suite to validate expected editing behavior.
Changes:
- Add the
runtime/pack/dist/opt/surroundpackage (plugin + autoload implementation). - Add help documentation for the package and document activation via
:packadd surroundin the user manual. - Add a new test script and register it in the test runner makefile lists.
Reviewed changes
Copilot reviewed 4 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/testdir/test_plugin_surround.vim | New regression tests covering add/remove/change surround across modes/selections |
| src/testdir/Make_all.mak | Registers the new surround test in NEW_TESTS/NEW_TESTS_RES |
| runtime/pack/dist/opt/surround/plugin/surround.vim | Package entrypoint: imports autoload and defines <Plug> + default mappings |
| runtime/pack/dist/opt/surround/autoload/surround.vim | Core surround implementation (Add/Remove/Change + helpers) |
| runtime/pack/dist/opt/surround/doc/surround.txt | Help documentation for the new package |
| runtime/pack/dist/opt/surround/doc/tags | Pre-generated helptags for the package help file |
| runtime/doc/usr_05.txt | User manual section describing how to enable the optional surround package |
| Filelist | Adds the new runtime files to the distributed runtime file list |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
My weak opinion is that distributed plugins like this are a bit questionable. If the case is strong enough to distribute them, it can't be much more of a step to consider them for inclusion as core features. Including plugins does also afford us the opportunity to rethink the feature design based on experience with the long standing external plugins rather than simply maintaining compatibility with them. If we can't come up with something more compelling and integrated with Vim I don't really see the point. This is not intended as an objection to the merging of this PR. |
|
I am against merging this PR. I believe most users use plugin managers to install third-party plugins like the ones mentioned at the beginning (though I rarely use them myself). I don't see much value in bundling a similar plugin with Vim core now. Wouldn't doing so discourage competition among third-party plugins? Also, while I'm not sure if this added plugin is something that has already been well-refined and stabilized elsewhere, or if it was newly written just for this PR, wouldn't it place a heavy maintenance burden on you, @habamax? Even if maintenance isn't an issue, wouldn't it be better if you just published it individually as a third-party plugin? |
|
What is the criteria of accepting a plugin in vim? Vim already bundled some optional plugins that have third-party plugins before they were accepted:
This plugin is quiet a bit of code, it could be a heavy maintenance burden. Maybe it's better to maintain it outside of vim. I use surround a lot and I am happy to see it bundled in vim. Publishing as a third-party plugin is fine too. |
It is not meant to be something to discourage competition. The goal is to have surround operations in vim available more or less out of the box. This optional plugin has basic add/remove/change operations that should cover most of the use-cases, however, if anything more advanced is needed users can install other plugins that can do more. Same as with comment package.
I am fine with the maintenance.
It wouldn't as it contradicts the idea of having surround operations available in vim out of the box. P.S. |
surround.txt -> pi_surround.txt
There are 2 very well known and established plugins to surround text in vim:
However it might be good to have one in the vim itself.
This package can do most if not all of what
surround.vimcan and by default uses the sameys,dsandcsoperators,Sfor visual mode. It is less fancy thanvim-sandwichwhich provides quite a few additional nice things.Key difference is implementation, I use 3 simple operators (add, remove and change surround) that are naturally dot repeatable without using
.mapping or repeat.vim.Additionally, adding surround is different:(UPD: it is the same asys(iwvsysiw((surround.vim) vssaiw((vim-sandwich), basically the move comes the last.surround.vim--ys{motion}{char}I will add tests in either case (whether this would or wouldn't be accepted).Tests are added.