kylechui / nvim-surround

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

fix: Add final HTML tag angle bracket only once #82

Closed Gelio closed 2 years ago

Gelio commented 2 years ago

Hey! First of all, thanks for this great plugin :tada: I appreciate that I can hack away at surrounding objects using Lua and not Vimscript :sweat_smile:

As a contributor, I appreciate that this plugin has tests which I could easily extend to add a failing test case for the bug I found. The Lua code seems to also be well-structured and I could easily find the place I had to change to fix the bug.


I noticed a bug when surrounding with t<div class="test"> - the final angle bracket was inserted twice. The final output was:

<div class="test">>some text</div>

instead of

-<div class="test">>some text</div>
+<div class="test">some text</div>

I decided to add tests for surrounding with HTML tags (one of which was failing because of this bug), and then fix it in another commit.

The problem seems to be due to (.*) (in the (.*)>? regular expression) eagerly including the final > of the input inside of the .* matching group. By using [^>] for the attributes instead of .*, the final angle bracket is correctly matched by >?.

Tests result

```sh 10:36 $ nvim --headless -c "PlenaryBustedDirector tests" Starting...Scheduling: tests/surround_spec.lua ======================================== Testing: /home/voreny/.local/share/nvim/site/pack/packer/start/nvim-surround/tests/surround_spec.lua Success || nvim-surround can be setup without a table Success || nvim-surround can surround text-objects Success || nvim-surround can delete surrounding quotes/parens Success || nvim-surround can change surrounding delimiters Success || nvim-surround can delete quotes using aliases Success || nvim-surround can modify surrounds that appear at 1, 1 Success || nvim-surround can dot-repeat insertions Success || nvim-surround can dot-repeat deletions Success || nvim-surround can dot-repeat changes Success || nvim-surround can dot-repeat deletions (aliased) Success || nvim-surround can dot-repeat changes (aliased) Success || nvim-surround can do the demonstration Fail || nvim-surround can visual-line surround .../pack/packer/start/nvim-surround/tests/surround_spec.lua:26: Expected objects to be the same. Passed in: (table: 0x7f4cc947a198) { [1] = '{' *[2] = ' 'hello',' [3] = '}' [4] = ''hello',' [5] = ''hello',' } Expected: (table: 0x7f4cc9479f80) { [1] = '{' *[2] = ' 'hello',' [3] = '}' [4] = ''hello',' [5] = ''hello',' } stack traceback: .../pack/packer/start/nvim-surround/tests/surround_spec.lua:26: in function 'check_lines' .../pack/packer/start/nvim-surround/tests/surround_spec.lua:197: in function <.../pack/packer/start/nvim-surround/tests/surround_spec.lua:189> Success || nvim-surround can deal with different whitespace characters Success || nvim-surround can insert user-inputted delimiters Success || nvim-surround can surround with an HTML tag div Success || nvim-surround can surround with an HTML tag
Success || nvim-surround can surround with an HTML tag
Success || nvim-surround can dot-repeat user-inputted delimiters Success || nvim-surround can jump to the deletion properly Success || nvim-surround can dot-repeat jumps properly Success || nvim-surround can handle quotes smartly Success || nvim-surround can delete close/empty pairs Success || nvim-surround can disable default delimiters Success || nvim-surround can modify aliases Success: 27 Failed : 1 Errors : 0 ======================================== ```

The one failing test is due to vim.o.shiftwidth = 2 in my init.lua. It is fixed by #81

Minor suggestions

While contributing, I noticed that:

  1. There is not formatter configured for this project. By default, StyLua (which I run on save) was formatting the code in a different way than it is formatted at the moment. This was mildly annoying since I had to :noa w or disable the auto-formatting on save. Maybe we should have a formatter (e.g. stylua) configured for this project (and maybe also verify it in CI)?
  2. The tests are not run in GitHub Actions

both of these seem like easy chores that would make it more convenient for other contributors.

NoahTheDuke commented 2 years ago

I'm just an observer and new to Lua: what changed when formatted with Stylua?

kylechui commented 2 years ago

@Gelio Thanks for the PR! I'll be merging this into main shortly. As for @NoahTheDuke, the point of using StyLua with the project would be to enforce a consistent code style throughout the code base via a stylua.toml, rather than have the code style change every single time someone formats with their own local formatter (causing the diffs to be polluted with nonsense). I'm currently looking into setting up StyLua for myself (and the project).

I don't have any experience with GitHub actions, but I'll look into that as well as a way to help people get started with contributing code (to be fair, this is only the second PR that's not from me haha)

kylechui commented 2 years ago

I've just added a stylua.toml to the project, so hopefully that should address the first of your concerns.

Gelio commented 2 years ago

Thanks for merging the MR and configuring StyLua! :tada:

kylechui commented 2 years ago

@Gelio I think the automatic testing will have to be tabled for now, since there are still a few "more pressing" matters (e.g. the remaining issues, writing more comprehensive tests, documentation) but I'll take a look at using GH actions for running the tests in the future. Thanks again for the PR and suggestions!

Gelio commented 2 years ago

No problem! I appreciate your attitude and thanks for this great plugin :smile: