Swaparound Feature

[mlysle]

This PR extends the swap feature with the optional ability to "swaparound" i.e. it can swap (x, y, z) -> (z, y, x). The option is disabled by default.

It might be hard to read the commits because my formatter changed all the spaces to tabs in my initial commits. Most of the actual changes happen in the left and right swap functions in swap.lua. In addition, I defined a new function in nodes.lua to find the "farthest" node.

[aaronik]

So I'm confused about what this is, can you explain more?

[mlysle]

It lets you swap non-adjacent nodes. In the current main if you run :Treewalker SwapLeft or :Treewalker SwapRight and the current node does not have a sibling in the direction you are trying to swap, then nothing happens. In my fork, there is a new option "swaparound". With swaparound is enabled, if no target is found, it will try to set the target to the "farthest sibling". So if you are looking at (x, y, z) and you try to swap x to the left, it will target z.

[aaronik]

I see, ok thanks for the explanation.

The idea sounds cool. Honestly, the behavior of not swapping around was never explicitly defined. And the core concept doesn't seem particularly difficult to maintain. So honestly, I'd be down to just bring this in as core functionality - no config option at all.

Howevery, it needs some things first:

1. Testing. There's a test suite in here that covers all the functionality of the plugin. No new functionality comes in until it's tested in many languages and edge cases are explored.
2. All the types are explicit (looking at farthest_sibling currently)
3. Commits get squashed

If all that stuff is included, and the config option is removed, then I'll be happy to merge this 🙂

[mlysle]

Cool, thanks for the feedback. In terms of code changes I've done everything you asked for. (edit: noticed I forgot to remove one line, will get to that later) Looking into testing next, will report what I find.

[aaronik]

Awesome possum! Let me know if there's anything I can do to clarify.

Please note - this codebase is a codebase. And the tests are too. So, they're not perfect. At first I started grouping things together by action, like the highlight spec, and there was a movement spec. But it started getting so huge, I realized it probably should be broken up by the filetype it's testing. So forgive the leftovers!

[mlysle]

OK so started writing those tests starting with one for lua. I guess I can skip languages like markdown where left/right swaps are disabled.

[mlysle]

I'm still working on this. It's not difficult to add more tests, but I can't run any tests afterr HTML because "make test" fails there and doesn't proceed to further test. This has nothing to do with any changes that I've introduced on my feature branch because it happens for me on the main branch as well.

Testing: 	/home/mally/treewalker.nvim/tests/treewalker/html_spec.lua	
Success	||	In an html file ::The test suite has the [html/] parser::	
Success	||	In an html file ::The test suite has the [javascript/html] parser::	
Fail	||	In an html file doesn't stop on footers	
            ./tests/treewalker/helpers.lua:38: expected to be at [22/5](|<main>) but was at [53/5](|<script>)
            Expected objects to be the same.
            Passed in:
            (table: 0x7f8f28a3a488) {
             *[1] = 53
              [2] = 5 }
            Expected:
            (table: 0x7f8f28a3a428) {
             *[1] = 22
              [2] = 5 }
            
            stack traceback:
            	./tests/treewalker/helpers.lua:38: in function 'assert_cursor_at'
            	/home/mally/treewalker.nvim/tests/treewalker/html_spec.lua:16: in function </home/mally/treewalker.nvim/tests/treewalker/html_spec.lua:13>

[aaronik]

Wow that's a bizarre failure - it's not failing on my machine or on the CI at all, so it might be something with your build. Not sure what versions of nvim or treesitter you got going on, but those might have something to do with it.

[aaronik]

This is looking pretty good so far, just need:

• Remove the whitespace and miscellaneous changes that aren't relavent to this feature
• Remove the swaparound part from the Opts in init.lua
• Add a test for python, ruby, and lua

If you keep getting stuck on this failing html test, you can modify the Makefile's test target and set keep_going = false to true instead of false, then the whole suite will keep going even when it encounters a failure.

[mlysle]

Alright, I believe this feature is mergeable now.

[aaronik]

Can we remove all these -- (|node1, node2) comments where they're not accurate? Otherwise this is ready for merge!

[mlysle]

Are they inaccurate? In that example, moving your cursor to 52, 31 would put your cursor right before "int a" in public static int Add(int a, int b) => a + b; where "int a" is node1 and "int b" is node 2.

[aaronik]

Yes they originally come from the lua spec files:

rg "node1, node2"

lua/treewalker/nodes.lua
82:function M.have_same_srow(node1, node2)
90:function M.have_neighbor_srow(node1, node2)
101:function M.have_same_scol(node1, node2)

tests/treewalker/lua_spec.lua
193:      "local function have_same_range(node1, node2)"
212:      "local function have_same_range(node1, node2)"
218:    assert.same("local function have_same_range(node1, node2)", lines.get_line(38))
226:    assert.same("local function have_same_range(node1, node2)", lines.get_line(38))
285:    vim.fn.cursor(38, 32) -- (|node1, node2)
305:    vim.fn.cursor(38, 32) -- (|node1, node2)

tests/fixtures/lua.lua
38:local function have_same_range(node1, node2)

[mlysle]

Ohh, I didn't realize node1/node2 were literal parameter names. I'll remove those comments.

[mlysle]

Removed those comments.

[aaronik]

Was about to merge then caught one last thing, see the above comment. This is super close though!

[aaronik]

Hey so I pulled down your branch and tested out the functionality, and saw this test in particular. This is the only one with more than 2 parameters. At first I thought it was perfect but when I played with it, the swapping behavior isn't quite how I expect it to be. The way it is now the first and last items swap. This leads to these really interesting reorderings if you cycle through - a fascinating mathematical phenomenon, but not the desired behavior. What should happen is, if the node is at the end, it should just move itself to the other end.

Here, use these for the haskell tests:

  it("swaparound behavior works", function()
    vim.fn.cursor(40, 15) -- [|1, 3, 5, 7, 9]
    assert.same("  printEvens [1, 3, 5, 7, 9]", lines.get_line(40))
    tw.swap_left()
    assert.same("  printEvens [3, 5, 7, 9, 1]", lines.get_line(40))
    tw.swap_left()
    assert.same("  printEvens [5, 7, 9, 1, 3]", lines.get_line(40))
    tw.swap_left()
    assert.same("  printEvens [7, 9, 1, 3, 5]", lines.get_line(40))
    tw.swap_left()
    assert.same("  printEvens [9, 1, 3, 5, 7]", lines.get_line(40))
    tw.swap_left() -- back to original order
    assert.same("  printEvens [1, 3, 5, 7, 9]", lines.get_line(40))

    vim.fn.cursor(40, 27) -- [|1, 3, 5, 7, |9]
    assert.same("  printEvens [1, 3, 5, 7, 9]", lines.get_line(40))
    tw.swap_right()
    assert.same("  printEvens [9, 1, 3, 5, 7]", lines.get_line(40))
    tw.swap_right()
    assert.same("  printEvens [7, 9, 1, 3, 5]", lines.get_line(40))
  end)

[mlysle]

Running into a little trouble here. I've rewritten the code to "reorder" sibling nodes by iteratively swapping, and it works for the most part. However, it cannot swap tables whether in Haskell or any other language.

The function in question:

---@param node TSNode
---@param fn function
function M.reorder(node, fn)
  if not node then return end
  if not fn then return end

  ---@param iter TSNode
  local iter = fn(node)
  while iter do
    operations.swap_nodes(node, iter)
    node = iter
    iter = fn(iter)
  end

  vim.fn.cursor(
    nodes.get_srow(node),
    nodes.get_scol(node)
  )
end

What seems to go wrong is that after running operations.swap_nodes() once, fn(iter) becomes nil so the loop does not continue. This only happens with tables and I'm not sure why.

While trying other methods, I found a way to do this by swapping "blindly" by collecting first, if that makes sense, however I think the above method is more readable.

[aaronik]

Hmmm that's a cool approach - but I worry about calling swap a bunch of times - that would overload users' undo list. Instead, maybe we can dispense with the idea of swapping two nodes entirely, and just remove the one node, then put it somewhere else? Is that feasible at all? Otherwise if that doesn't make sense, maybe your original solution is good, but with a bounds check, and if it's at the end, then it should just place the node at the beginning/end of the list? Maybe that's the same as the first suggestion..