feat(extras): Insert repeat node when duplicated keys occur

[uyha]

Experimental implementation for #465

[L3MON4D3]

Nice so far 👍
I'd like to see the access to node.pos wrapped in a function, like node.get_jump_indx. (I think this should make clear that the jump-index may not be changed)

[uyha]

Hmmm, isn't it better to stick with pos for now and check if pos is nil or not to decide between copy3 and rp? or do you have in mind that get_jump_index can be used in other use cases?

[L3MON4D3]

Hmmm, isn't it better to stick with pos for now and check if pos is nil or not to decide between copy3 and rp?

Oh, yeah that's what I also had in mind, I only think we should wrap the access to pos in a function (so the nodes implement a new function node:get_jump_indx() which returns node.pos)
Both because pos isn't very informative (could be the jump-index, or the position in the buffer, or the position of the node in its parent), and because it feels like a function is the nicer interface.

do you have in mind that get_jump_index can be used in other use cases?

Not really, but I'd like the extras to not rely on any internals of luasnip, only public API.

[uyha]

I added the function, can you have another look?

[L3MON4D3]

Yup 👍
There's one issue with the tests: they expect numbers (for simplicity, and because it used to be pretty generic), but now we need to make it work with nodes (or objects that pretend to be nodes, eg. implement exactly the functionality we need in interpolate).
I think the best way to fix that is just mocking the nodes, and adding a __tostring-metamethod to still get the expected strings from table.concat.

Aside from that, just docs are left, and some tests for the new functionality.
Are you familliar with neovim's test-framework?

[uyha]

I am not familiar with it, but I will take a look.

[uyha]

so, i am getting to mocking the nodes, but i am not sure how to actually do it since it requires the node definition to be global and I am not sure how to proceed with that.

[L3MON4D3]

Oh, I think it's enought to only mock the functions we actually need (so just get_jump_indx, and I think that can just always return nil).
Since those tests are mainly about making sure the interpolation is correct, we only need to simulate nodes so far that it actually works, but no further.

For actual tests of the functionality, I'd say we can test it in a real setting, I feel like that's easiest to write tests for too (use screen:snapshot_util() to generate a capture of the tested neovim-instance, and test it via screen:expect (bunch of examples for that in tests/integration))

[uyha]

but the problem i have here is not knowing how to pass a mock object to the works call, I tried putting

local FakeNode = {}
function fake_node() end

at the beginning of fmt_spec.lua and have a call like

works(“”, “{a}{a}”, “{a = fake_node()}”, “”, “{repeat_duplicates = true}”)

I know that it shouldn’t work but for the reason of the test case being wrong, here the test case fails due to fake_node is not visible to the caller.

[L3MON4D3]

Ahhh, you can run code in the tested neovim-instance via exec_lua, defining fakeNode with that should work

[uyha]

I added a test, it is messy but I want to see if the direction is correct first. Also, it is a bit weird since tests like this won't tell if there repeat is being used or not.

[L3MON4D3]

That test looks good👍👍
Can you make it generic, so the existing tests can be re-used? (ie put part of the new test into the works-function)

Also, it is a bit weird since tests like this won't tell if there repeat is being used or not

Yeah, it's unfortunate that the current tests for just the interpolation rely on this function handling objects other than nodes :/
The actual new tests for this feature should probably be integration-tests (so create a snippet with repeated nodes, expand it, and check that everything works as expexted), at least that's how I like to test most features.

[uyha]

I generalize the works function, but there is a problem. Since interpolate's result will have a repeat node which doesn't have a __tostring, so it can't be stringified which leads to the test failing. Any idea how i should proceed?

[uyha]

The previous code works because I have a bug in the mock table btw

[L3MON4D3]

I generalize the works function, but there is a problem. Since interpolate's result will have a repeat node which doesn't have a __tostring, so it can't be stringified which leads to the test failing. Any idea how i should proceed?

Turn of repeat-insertion for those tests? That should be fine as long as we check it elsewhere

[uyha]

I am not sure what you mean by turning off repeat-insert here

[L3MON4D3]

Ah, repeat_duplicates to false

[uyha]

oh, ok

[L3MON4D3]

I took the liberty of implementing my idea of handling the existing tests (figured explaining would take longer than quickly handling it myself 😅) WDYT?

[uyha]

I was trying to make it as general as possible since I am not sure how you want it, but this is simpler in my opinion. So, I should move on to create the integration test, right?

[L3MON4D3]

Ah, I might've been a bit misleading then, I only wanted to re-use those tests (but they're also more readable now, with the explicit argument-names, so that's nice :D)

Umm yeah, integration tests sound good👍👍

[L3MON4D3]

Oh great idea to forward TEST_FILE 👍

[uyha]

yeah, it was annoying having to run the whole test set everytime

[uyha]

I added 2 tests, 1 one for fmt and 1 for rep, both of them are failing but I'm not sure why since testing on real nvim instance works fine

[L3MON4D3]

Ah, screen:expect is very particular about the text it receives :/
Try formatting the text there like in the passing tests (especially whitespace!)

[uyha]

that’s not really the problem, the problem is the repeating text doesn’t appear at all in the actual portion of the screen 😅

[uyha]

Expected:
  |*asdf^ repeat asdf                                  |
  |{0:~                                                 }|
  |{2:-- INSERT --}                                      |
Actual:
  |*asdf^ repeat                                       |
  |{0:~                                                 }|
  |{2:-- INSERT --}                                      |

[L3MON4D3]

Ahhh xD
Okay that's probably due to updateevents being different, try jumping after feeding the text (jump always causes an update)

[uyha]

eh, how do you do a jump in this case 😅?

[L3MON4D3]

Oh, manually, binding some keys to jump is not necessary: exec_lua(ls.jump())

[uyha]

I think it is done now, could you have another look?

[L3MON4D3]

Nice nice nice, this is it, great work!! :D
Would you squash all of this into two commits, one for the fmt-stuff, one for the makefile?

[uyha]

sure

[uyha]

can you have a look again?

[uyha]

welp, looks like either github action's internet is down or neovim-ppa is down

[L3MON4D3]

off and on again did it 👍

[uyha]

hmmm, it seems like i messed up the commits, i will fix it

[L3MON4D3]

Ah my bad, I just noticed that get_jump_index should be officially exported -.-
Best add a subsection Api (like in Snippets below) here

[uyha]

everything should be good now, could you have another look?

[L3MON4D3]

Ohh right, just Api doesn't work, then that tag is duplicated.. Could you change the titles to Snippet-Api and Node-Api?

[uyha]

looks like everything is in order

[L3MON4D3]

I agree 👍👍
Nice work, thank you very much for sticking through all of my demands 😅

[uyha]

not a problem, thank you very much for the plugin

[danielo515]

This is awesome. Thanks