Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[RFC] Restore node #228

Merged
merged 27 commits into from
Nov 30, 2021
Merged

[RFC] Restore node #228

merged 27 commits into from
Nov 30, 2021

Conversation

L3MON4D3
Copy link
Owner

@L3MON4D3 L3MON4D3 commented Nov 27, 2021

This implements a new type of node, the restoreNode.
It allows sharing of user-entered data (text in an insertNode, changed choices, ..) between multiple nodes.

local luasnip = require("luasnip")
local c = luasnip.choice_node
local t = luasnip.text_node
local i = luasnip.insert_node
local r = luasnip.restore_node
local s = luasnip.snippet
local s = luasnip.snippet_node


s("rest", {
	c(1, {
		sn(nil, {t('"'), r(1, "key"), t('"')}),
		sn(nil, {t("'"), r(1, "key"), t("'")}),
	})
}, {
	stored = {
		key = sn(nil, {i(1, "preset")})
	}
}),

The snippet will start out as "preset", changing the text to eg. "something else" and changing choice will result in 'something else'.
The restoreNode can also be used to preserve text in insertNodes inside of a snippet generated by dynamicNode accross multiple updates of that snippet.

Right now only snippetNodes can be stored, though that shouldn't be too annoying, we can always wrap other nodes inside snippetNodes.

@L3MON4D3
Copy link
Owner Author

One thing that I'm not sure about is having to define the snippetNodes in the opts for the snippet, which may be far away from their usage.

@L3MON4D3 L3MON4D3 changed the title Restore node [RFC] Restore node Nov 27, 2021
@stasjok
Copy link
Contributor

stasjok commented Nov 27, 2021

I just tried it for very basic cases. I will respond later, when I tried more complicated snippets.

  • It doesn't work after commit feat: restoreNode gets its own type.
  • Probably something wrong with the docstring. I have an error Error executing vim.schedule lua callback: .../pack/packer/start/LuaSnip/lua/luasnip/nodes/snippet.lua:635: attempt to index local 'node_text' (a nil value) after selecting a snippet with nvim-cmp.
  • I think it's better by default to have empty input node instead of empty text node. Most of the time we want to preserve user-edited text, so I think it should be inputNode.

One thing that I'm not sure about is having to define the snippetNodes in the opts for the snippet, which may be far away from their usage.

Some hacky solution can be to add third argument to restore_node: default snippet. I'm not sure about the order in which they are evaluated, but it can look like this (though I'm not sure if it's better, because it is not explicit):

s("rest", {
	c(1, {
		sn(nil, {t('"'), r(1, "key", sn(nil, {i(1, "preset")})), t('"')}),
		sn(nil, {t("'"), r(1, "key"), t("'")}),
	})
}

AFAIK first choice is always evaluated first. So default should be provided there. But it shouldn't replace stored table, just another way for setting this up.

@L3MON4D3
Copy link
Owner Author

It doesn't work after commit feat: restoreNode gets its own type.

Yeah, debugging it right now :|

Concerning the docstring, jep, that also doesn't work yet 😅
Having an empty insertNode sounds more sensible too 👍

@L3MON4D3
Copy link
Owner Author

Should work again, jumpable nodes are found via their type and I forgot to add restoreNode in that check.

@L3MON4D3
Copy link
Owner Author

Docstrings might work now 😅
Another example, this time for dynamicNode:

local function simple_restore(args, _)
	return sn(nil, {i(1, args[1]), r(2, "dyn")})
end

s("rest", {
	i(1, "preset"), t{"",""},
	d(2, simple_restore, 1)
}, {
	stored = {
		dyn = i(1, "will be stored")
	}
}),

Normally the text in r(2) would be reset to ""(or would have to be manually preserved), but here it is done automatically.

@L3MON4D3
Copy link
Owner Author

One thing that I'm not sure about is having to define the snippetNodes in the opts for the snippet, which may be far away from their usage.

Some hacky solution can be to add third argument to restore_node: default snippet. I'm not sure about the order in which they are evaluated, but it can look like this (though I'm not sure if it's better, because it is not explicit):

s("rest", {
	c(1, {
		sn(nil, {t('"'), r(1, "key", sn(nil, {i(1, "preset")})), t('"')}),
		sn(nil, {t("'"), r(1, "key"), t("'")}),
	})
}

AFAIK first choice is always evaluated first. So default should be provided there. But it shouldn't replace stored table, just another way for setting this up.

Yeah, that would also work, we could specify that if exactly one default is given, that one is used, if there's multiple it's undefined.
I think it would look better if we just accept a single node/table of nodes and turn it into a snippetNode in the constructor.

If the default is specified more than once it's undefined which one is
actually used.
If the default is set within a dynamicNode, it will not influence
restoreNodes outside the dynamicNode directly after expansion. The
reason for this is that dynamicNodes can only be constructed (and their
subsnip_init() called) after put_initial() has been called for all of
the argnodes (actually all the nodes, with more effort this could be
optimized to just the argnodes).
@L3MON4D3
Copy link
Owner Author

The default may now be set directly inside the snippet:

local function simple_restore(args, _)
	return sn(nil, {i(1, args[1]), r(2, "dyn", {
		i(1, "the node can also be set here"),
		t{"",""},
		i(2, "some more text")
	})})
end

local function simple_restore2(args, _)
	return sn(nil, {i(1, args[1]), r(2, "dyn", i(1, "the node can also be set here"))})
end

s("rest", {
	i(1, "preset"), t{"",""},
	d(2, simple_restore, 1)
}),

@stasjok
Copy link
Contributor

stasjok commented Nov 28, 2021

I'm sorry for a long response. I tested it more thoroughly, but I was doing it before your last commit ("Allow the default for a given key to be passed to the constructor"). Also I didn't test dynamicNodes at all. I will try to test more tomorrow.

Firstly, thank you! Basically it works, it's great!

Some things I notices aren't working as I expected (remember it doesn't account for the latest commit):

  • I expected, that it takes exactly one node (like choice node) with nil as index if node is jumpable. So sn(nil, {nodes}) or i(nil, "default"). Now if it's a single node, I need to specify index 1. In latest commit you allowed it to be a list of nodes, right? Didn't test it, but in that case it can be valid point only for a single node, not for a table of nodes.
  • It didn't work for me, if I tried to use restore_node without wrapping it in a snippet_node, but I would think it should be supported: c(1, { r(nil, "key"), sn(nil, { t("prefix "), r(1, "key") }) })
  • Any transformation functions/repeats/lambdas aren't resolved immediately. I need to change text for them to work.

Here is a long example of what I tested:

local ls = require("luasnip")
local s = ls.snippet
local sn = ls.snippet_node
local t = ls.text_node
local i = ls.insert_node
local f = ls.function_node
local c = ls.choice_node
local d = ls.dynamic_node
local l = require("luasnip.extras").lambda
local rep = require("luasnip.extras").rep
-- local p = require("luasnip.extras").partial
-- local m = require("luasnip.extras").match
-- local n = require("luasnip.extras").nonempty
-- local dl = require("luasnip.extras").dynamic_lambda
-- local fmt = require("luasnip.extras.fmt").fmt
-- local fmta = require("luasnip.extras.fmt").fmta
-- local types = require("luasnip.util.types")
-- local conds = require("luasnip.extras.expand_conditions")
local r = ls.restore_node

ls.snippets = {
  all = {
    -- Just for testing
    s("test_empty_stored", {
      c(1, {
        sn(nil, { t('"'), r(1, "text"), t('"') }),
        sn(nil, { t("'"), r(1, "text"), t("'") }),
      }),
    }),
    s("test_single_input_in_stored", {
      c(1, {
        sn(nil, { t("("), r(1, "text"), t(")") }),
        sn(nil, { t("["), r(1, "text"), t("]") }),
        sn(nil, { t("{"), r(1, "text"), t("}") }),
      }),
    }, {
      stored = { text = i(1, "default_text") }, --< I expect it to be i(nil, default_text), like in choiceNode
    }),
    s("test_restore_in_second_choice", {
      c(1, {
        t("irrelevant_text"),
        sn(nil, {
          t("name: "),
          r(1, 1),
        }),
        sn(nil, {
          t("dest: "),
          r(1, 1),
        }),
      }),
    }, {
      stored = {
        [1] = i(1, "default_name"),
      },
    }),
    s("test_bare_resore", {
      c(1, {
        r(nil, 1), --< doesn't work without wrapping it in snippetNode
        sn(nil, {
          t("name: "),
          r(1, 1),
        }),
      }),
    }, {
      stored = {
        [1] = i(1, "default_name"),
      },
    }),
    s("test_multiple_inputs_in_restore", {
      t("Prefix: "),
      c(1, {
        sn(nil, { r(1, 1) }),
        sn(nil, { r(1, 1), t(", "), i(2, "extra_input") }),
      }),
    }, {
      stored = {
        [1] = sn(nil, {
          t("Some "),
          i(1, "complicated"),
          t(" input with multiple "),
          i(2, "nodes"),
          t(". Also repeat: "),
          rep(2), --< repeat doesn't work immediately (need to change text)
        }),
      },
    }),
    s("test_repeat_inside_snippet_node", { --< just to be sure it should work
      i(1, "Input1"),
      t(" "),
      i(2, "Input2"),
      t(" "),
      sn(3, {
        i(1, "Cloned_input"),
        t(" "),
        rep(1),
      }),
    }),
    s("test_multiple_stored", {
      c(1, {
        sn(nil, { r(1, 1), t(":"), r(2, 2) }),
        sn(nil, { r(1, 1), t(":"), r(2, 3) }),
        sn(nil, { r(1, 2), t(":"), r(2, 3) }),
      }),
    }, {
      stored = {
        [1] = i(1, "simple input"),
        [2] = sn(nil, {
          t("<"),
          i(1, "wrapped_input"),
          t(">"),
        }),
        [3] = sn(nil, {
          t("a="),
          i(1, "1"),
          t(" b="),
          i(2, "2"),
        }),
      },
    }),
    s("test_stored_inside_stored", {
      c(1, {
        sn(nil, { t("-->"), r(1, "separated_with_space"), t("<--") }),
        sn(nil, { r(1, "separated_with_newline") }),
      }),
    }, {
      stored = {
        [1] = sn(nil, { t("First: "), i(1, "input1") }),
        [2] = sn(nil, { t("Second: "), i(1, "input2") }),
        ["separated_with_space"] = sn(nil, { --< Wow, didn't expect it, but it works
          r(1, 1),
          t(" "),
          r(2, 2),
        }),
        ["separated_with_newline"] = sn(nil, {
          r(1, 1),
          t({ "", "" }),
          r(2, 2),
        }),
      },
    }),
    s("test_stored_with_lambda", {
      i(1, "Pretext"),
      t(" "),
      c(2, {
        sn(nil, r(1, 1)),
        sn(nil, {
          i(1, "extra_arg"),
          t(" "),
          r(2, 1),
        }),
      }),
    }, {
      stored = {
        [1] = sn(nil, {
          i(1, "reversed text"),
          t(" "),
          l(l._1:reverse(), 1), --< lambdas and functions work only after changing text
        }),
      },
    }),
    s("test_stored_with_funcion", {
      i(1, "Pretext"),
      t(" "),
      c(2, {
        sn(nil, r(1, 1)),
      }),
    }, {
      stored = {
        [1] = sn(nil, {
          i(1, "prefixed text"),
          t(" "),
          f(function(args) --< lambdas and functions work only after changing text
            return "Prefix:" .. args[1][1]
          end, 1),
        }),
      },
    }),
    -- Some complicated example of something I wanted to try to do at the first place
    -- Ansible module `file`: https://docs.ansible.com/ansible/2.9/modules/file_module.html
    -- Other uses is to create snippets with minimum always used options,
    -- but always can switch to a form with more optional parameters
    s("file", {
      t({ "file:", "\t" }),
      c(1, {
        sn(nil, {
          r(1, "path"),
          r(2, "permissions_dir"),
          t({ "", "\tstate: directory" }),
        }),
        sn(nil, {
          r(1, "path"),
          r(2, "src"),
          t({ "", "\tstate: link" }),
        }),
        sn(nil, {
          r(1, "path"),
          t({ "", "\tstate: absent" }),
        }),
        sn(nil, {
          r(1, "path"),
          r(2, "src"),
          t({ "", "\tstate: hard" }),
        }),
        sn(nil, {
          r(1, "path"),
          r(2, "permissions_file"),
          t({ "", "\taccess_time: " }),
          i(3, "preserve"),
          t({ "", "\tmodification_time: " }),
          i(4, "preserve"),
          t({ "", "\tstate: touch" }),
        }),
        sn(nil, {
          r(1, "path"),
          r(2, "permissions_file"),
          t({ "", "\tstate: file" }),
        }),
      }),
    }, {
      stored = {
        path = sn(nil, {
          t("path: "),
          i(1, "/path/to/file"),
        }),
        permissions_file = sn(nil, {
          t({ "", "\tmode: " }),
          i(1, "0644"),
          r(2, "owner"),
        }),
        permissions_dir = sn(nil, {
          t({ "", "\tmode: " }),
          i(1, "0755"),
          r(2, "owner"),
        }),
        owner = sn(nil, {
          t({ "", "\towner: " }),
          i(1, "root"),
          t({ "", "\tgroup: " }),
          i(2, "root"),
        }),
        src = sn(nil, {
          t({ "", "\tsrc: " }),
          i(1, "/path/to/file/to/link/to"),
        }),
      },
    }),
  },
}

@L3MON4D3
Copy link
Owner Author

Now if it's a single node, I need to specify index 1.

Yes, that's because it will be (silently) turned into a snippetNode. We could either make clearer what exactly is happening behind the scenes (put it in the docs) or just change nil to 1, because that's surely what was meant here.

c(1, { r(nil, "key"), sn(nil, { t("prefix "), r(1, "key") }) })

that looks like it should work... lemme check what's going wrong.

functionNodes I didn't fully consider, but I thought that they would "just work" ™️. It's probably missing some small step.

Also thanks for the big example file, really nice to test with 👍

@L3MON4D3
Copy link
Owner Author

Okay, I think all of your testcases work now :D

@stasjok
Copy link
Contributor

stasjok commented Nov 29, 2021

I've tested it one more time with new additions. I managed to break in only with dynamic lambda (its content is reset after every choice). It's probably expected because after every choice it is evaluated again. But maybe it's possible to preserve its content also? If not I don't think it's a blocker. I just got carried away with testing features outside of the initial request scope...

If nitpicking things, you can make specifying defaults consistent between stored table and restore_node. I mean with the same rules, e.g. it can be a) single snippet_node b) one or many nodes with index starting from 1. In particular to allow specifying list of nodes in stored.

Overall I think this feature is integrated nicely and in a spirit of luasnip.

Just for fun I tried to remove old_state from jdocsnip function to see how much it's shorter. I've used "arg" .. arg as the key, it is not perfect, because it will create unnecessary keys as you type. I could've used indexes, but it'll break if user swaps arguments.

local function jdocsnip(args)
  local nodes = {
    t({ "/**", " * " }),
    r(1, "desc", i(1, "A short Description")),
    t({ "", "" }),
  }

  -- At least one param.
  if string.find(args[2][1], ", ") then
    vim.list_extend(nodes, { t({ " * ", "" }) })
  end

  local insert = 2
  for _, arg in ipairs(vim.split(args[2][1], ", ", true)) do
    -- Get actual name parameter.
    arg = vim.split(arg, " ", true)[2]
    if arg then
      local inode = r(insert, "arg" .. arg, i(1))

      vim.list_extend(nodes, { t({ " * @param " .. arg .. " " }), inode, t({ "", "" }) })

      insert = insert + 1
    end
  end

  if args[1][1] ~= "void" then
    local inode = r(insert, "ret", i(1))

    vim.list_extend(nodes, { t({ " * ", " * @return " }), inode, t({ "", "" }) })
    insert = insert + 1
  end

  if vim.tbl_count(args[3]) ~= 1 then
    local exc = string.gsub(args[3][2], " throws ", "")
    local ins = r(insert, "ex", i(1))
    vim.list_extend(nodes, { t({ " * ", " * @throws " .. exc .. " " }), ins, t({ "", "" }) })
    insert = insert + 1
  end

  vim.list_extend(nodes, { t({ " */" }) })

  local snip = sn(nil, nodes)
  return snip
end

ls.snippets = {
  all = {
    s("test_stored_with_dyn_lambda", {
      i(1, "Pretext"),
      t(" "),
      c(2, {
        sn(nil, r(1, 1)),
        sn(nil, {
          i(1, "extra_arg"),
          t(" "),
          r(2, 1),
        }),
      }),
    }, {
      stored = {
        [1] = sn(nil, {
          i(1, "reversed text"),
          t(" "),
          dl(2, l._1:reverse(), 1), --< dynamic lambdas resets its content
        }),
      },
    }),
    s("test_dynamic", {
      d(6, jdocsnip, { 2, 4, 5 }),
      t({ "", "" }),
      c(1, {
        t("public "),
        t("private "),
      }),
      c(2, {
        t("void"),
        t("String"),
        t("char"),
        t("int"),
        t("double"),
        t("boolean"),
        i(nil, ""),
      }),
      t(" "),
      i(3, "myFunc"),
      t("("),
      i(4),
      t(")"),
      c(5, {
        t(""),
        sn(nil, {
          t({ "", " throws " }),
          i(1),
        }),
      }),
      t({ " {", "\t" }),
      i(0),
      t({ "", "}" }),
    }),
  },
}

@L3MON4D3
Copy link
Owner Author

L3MON4D3 commented Nov 29, 2021

I've tested it one more time with new additions. I managed to break in only with dynamic lambda (its content is reset after every choice)

Ha, I think I know what's wrong there :D

If nitpicking things, you can make specifying defaults consistent between stored table and restore_node. I mean with the same rules, e.g. it can be a) single snippet_node b) one or many nodes with index starting from 1. In particular to allow specifying list of nodes in stored.

Right, yes that should work the same everywhere 👍

Just for fun I tried to remove old_state from jdocsnip function to see how much it's shorter. I've used "arg" .. arg as the key, it is not perfect, because it will create unnecessary keys as you type. I could've used indexes, but it'll break if user swaps arguments.

Ouh, nice! I'll replace the current one in examples as the point of adding it there was showing as many features as possible, including some more is great!

@stasjok
Copy link
Contributor

stasjok commented Nov 29, 2021

Ouh, nice! I'll replace the current one in examples as the point of adding it there was showing as many features as possible, including some more is great!

Then there won't be anything about old_state though.

@L3MON4D3
Copy link
Owner Author

L3MON4D3 commented Nov 29, 2021

True, but I'm not even sure if old_state has any use beyond restoring insertNode text...
That was the reason for implementing it at least

@stasjok
Copy link
Contributor

stasjok commented Nov 30, 2021

Are you waiting for my approval?

Anyway, looks like indent_snippet_node considers restore_node as one line (or one node).

    s("test_indent_snippet_node", {
      isn(1, {
        t("# "),
        t({ "preline1", "preline2", "" }),
        r(1, 1, {
          t({ "line1", "line2", "" }),
          i(1, "input"),
        }),
      }, "# "),
    }),
    s("test_indent_snippet_node_double", {
      c(1, {
        sn(nil, {
          t("// "),
          isn(1, r(1, 1), "$PARENT_INDENT// "),
        }),
        sn(nil, {
          t("# "),
          isn(1, r(1, 1), "$PARENT_INDENT# "),
        }),
      }),
    }, {
      stored = {
        [1] = {
          t("* "),
          isn(1, {
            t({ "line1", "line2", "" }),
            i(1, "user_input"),
          }, "* "),
        },
      },
    }),

@stasjok
Copy link
Contributor

stasjok commented Nov 30, 2021

What do you think about using wrap_nodes_in_snippetNode function also for choice_node? It can make manual wrapping nodes in snippet_node optional.

@L3MON4D3
Copy link
Owner Author

Had to write some Docs, still :D

Concerning the indentSnippetNode, changing indent is problematic as the indent is very static right now (eg. calculated once at snippet expansion and put before any newlines in insert and textNode). As the restored nodes may have different indentation depending on where they are restored (eg. one restoreNode is at the default indent, the other inside an ISN with // prefixed), the current method would have to be revised (either store the indent string and prefix text when it is put into the buffer or remove the old indent before adding new indent).
I'll come back to that later and put a note in the docs for now (shouldn't be a big problem, ISN just makes prefixing blocks of text easier, not possible in the first place)

@L3MON4D3
Copy link
Owner Author

What do you think about using wrap_nodes_in_snippetNode function also for choice_node? It can make manual wrapping nodes in snippet_node optional.

Not a bad idea, that shouldn't hurt 👍

@L3MON4D3 L3MON4D3 merged commit a3b71bf into master Nov 30, 2021
Comment on lines +460 to +467
ss("rest", {
i(1, "preset"), t{"",""},
d(2, simple_restore, 1)
}),
("rest", {
i(1, "preset"), t{"",""},
d(2, simple_restore, 1)
}),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like double pasting to me.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whoops, yes you're right

@stasjok
Copy link
Contributor

stasjok commented Nov 30, 2021

Concerning the indentSnippetNode, changing indent is problematic as the indent is very static right now (eg. calculated once at snippet expansion and put before any newlines in insert and textNode). As the restored nodes may have different indentation depending on where they are restored (eg. one restoreNode is at the default indent, the other inside an ISN with // prefixed), the current method would have to be revised (either store the indent string and prefix text when it is put into the buffer or remove the old indent before adding new indent).

Got it, thank you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants