Skip to content
This repository has been archived by the owner on Jun 1, 2020. It is now read-only.

Goals & non-goals #2

Open
KillTheMule opened this issue Jul 20, 2018 · 37 comments
Open

Goals & non-goals #2

KillTheMule opened this issue Jul 20, 2018 · 37 comments

Comments

@KillTheMule
Copy link

KillTheMule commented Jul 20, 2018

Hey!

I'd like to discuss where this should be going eventually, so we can work out how the api should look like and such. I'm not intimately familiar with mappings, but I went through https://neovim.io/doc/user/map.html for some overview. Quite some things.

So, as a first goal I'd define this:

Provide a lua api to the map family where {rhs} is a lua function

If {rhs}, isn't a lua function, it's already pretty usable from vim.api.nvim_command. I assume we can limit ourselves to <Cmd> mappings, because the lua function can easily check for the mode and do nothing if inappropriate. Or is that assuming too much? Am I missing something?

So in it's simplest incarnation, the following should work imho (name of course non-final):

vim.lua_api.lua_map("<F2>", my_lua_function)

Thoughts:

  • See which of "buffer", "nowait", "silent", "script", "expr" and
    "unique" we need to incorporate, and in which way. My first intuition is an optional array of strings.
    • Maybe we should forego "" and make a different function vim.lua_api.buf_lua_map where you can choose the buffer (default current; maybe that's not great because "" works on the curbuf, so we'd need to change that temporarily).
    • "script" and "expr" don't make sense in our scope, right?
  • Do we need a variant of "SID" to prevent name conflicts? Not sure what that does when used from nvim_command. Same about "Plug".

You said something about reuseability, which is why you set up things the way you did. Can you elaborate on that? I did not really understand what you were after.

My pretty limited imagination says that the next step would be to provide something similiar for user commands, but let's take one step after the other :)

@hkupty
Copy link
Contributor

hkupty commented Jul 20, 2018

Thanks for coming up with a plan!

Basically, my idea with this one is not only to provide a bridge between mappings and lua functions, but to actually make lua functions being usable on places where we expect VimL functions.

Off the top of my head:

  • mappings
  • jobstart (and its variants)
  • commands
  • autocommands

My goal for this project is to create such mappings in a way that it'd feel natural to plugin developers, or better said, I want to hide the VimL bits of the integration from client plugins code.

Also, a complete success would be if such API ends up being adopted upstream, by neovim.


You said something about reuseability, which is why you set up things the way you did.

For example, imagine you have this single lua function:

local myfn = function(arg)
  -- do something awesome with arg
end

and, on vim side, you want to have:

" Map with empty, default arg
nmap <c-space> <cmd>lua <something>.myfn()<CR>o

" Command optionally accepts argument
command! -nargs=? Myfn lua <something>.myfn(<f-args>)

So that's where I said we should allow reusability. We need a binding name between lua and viml so we can call the indirection:

lua require("tycho").core.call("myfn", "arg")

If we don't provide a name, we'd need to come up with a name ourselves. That'd probably narrow down to random data, or - somehow - a hash of the function or a hash of the return of it based on some argument. None of those options are really good IMO, so I prefer an explicit naming to this attribute. Reusability comes for free.

A more subtle advantage of reusing is that if we see something like this:

map <space>1 <cmd>lua require("tycho").core.call("myfn")
map <space>2 <cmd>lua require("tycho").core.call("myfn", "extra-argument")

It is way simpler to understand what that mapping does than:

map <space>1 <cmd>lua require("tycho").core.call("4732098")
map <space>2 <cmd>lua require("tycho").core.call("65789423", "extra-argument")

Also, the same applies for reusing between mapping and command:

" Easy
command! -nargs=? Myfn lua require("tycho").core.call("myfn", <f-args>)
map <space>1 <cmd>lua require("tycho").core.call("myfn")

" Not so easy
command! -nargs=? Myfn lua require("tycho").core.call("myfn", <f-args>)
map <space>1 <cmd>Myfn

Sorry if this got too long. 😅

@KillTheMule
Copy link
Author

Off the top of my head:

mappings
jobstart (and its variants)
commands
autocommands

My goal for this project is to create such mappings in a way that it'd feel natural to plugin developers, or better said, I want to hide the VimL bits of the integration from client plugins code.

Ack. Just let's do mappings first :) I imagine jobstart should get some direct support from the API, so that might be more involved.

Also, a complete success would be if such API ends up being adopted upstream, by neovim.

That's definitely my target. It's also why I'm trying to limit the scope somewhat, because I think it's important to expose something to plugin developers fast to collect feedback.

... reusability...

Oh ok, so it's a global/local viml/lua names thing. I'm not sure I agree with the premises that is needed, though. For example, to use a lua function from viml, you can easily just export the lua module and make use of require.

The question is, how often is that stuff needed, do we need to add convenience? Do you have any "real" usecases why you'd want to do that? We could expose a function export_function_to_viml(fn, viml_name) so the user can choose the viml name.

@hkupty
Copy link
Contributor

hkupty commented Jul 20, 2018

just export the lua module and make use of require.

Let's think a bit:

Imagine plugin A having a function do_something.
A direct mapping would be something like:
map {keymap} <Cmd>lua require("plugin_a").do_something()

A possible mapping function could be:

tycho.map{
  package = "plugin_a",
  fn = "do_something",
  args = {}
}

However, we have a loose reference to the function. It can simply not exist or be renamed. The mapping will still be created. And the failure will be detected only when the user tries to use it. Specially because we won't be able to test this in runtime:

-- plugin_a.lua
local plugin = {}
plugin.do_something = function()
end

-- if this tests the function exists by `require(package)[fn] ~= nil`
-- we'd probably fail or recursively try to map this to infinity.
-- TBH, I don't know though
tycho.map{...}

return plugin

Of course, we have the same problem in the manual registering:

tycho.core.register("my_fn", plugin.do_something)
tycho.core.map("myfn", "...")

But we could just check the existence of such key on the mapping function.
Also, using the higher level API solves this:

tycho.myfn = plugin.do_something
tycho.api.map("...", tycho.myfn)

-- or with a random name
tycho.api.map("...", plugin.do_something)

In the end, I agree with you.. maybe an indirection layer is bad or makes things more complex.
But I don't want to have plugin developers or users having bad times figuring out what failed.

There are probably other pros and cons, but during this exercise, that's what came to my mind. wdyt?

@hkupty
Copy link
Contributor

hkupty commented Jul 20, 2018

We could expose a function export_function_to_viml(fn, viml_name) so the user can choose the viml name.

Yeah, that can work out.. still, we need to bind the function to some name so, even if we have a VimL function, the underlying implementation of that function can reach the provided lua anonymous function.

However, the bad thing about picking a random name is that we're not idempotent anymore. So if, for some reason, the function that registers in this binding table get's called by an autocommand handler, (i.e. every time a user opens a new buffer), we could cause undesired side effects (slowdown, memory starvation, idk..).

@KillTheMule
Copy link
Author

KillTheMule commented Jul 20, 2018

Specially because we won't be able to test this in runtime:

That's the "stringly type API" problem, right? I don't see why we couldn't test the existence of that function in the module. I think using require is safe here, see https://www.lua.org/pil/8.1.html. Or are you referring to something different?

The other way out is the way you're using it in your high-level API, namely accepting functions rather than function names. That would be my favourite, but needs some fleshing out. It'll be some hours still before I can do anything ..

Of course, we have the same problem in the manual registering:

I think this could be solved by having register return a special table that has to be used as the input to map. I would want to try to remove the need for registering though, seems like an additional step that's not fully needed.

However, the bad thing about picking a random name

Random names sound very bad to me, let's try to avoid that, if only for our own debugging sanity.

I guess an idea would by namespacing-by-neovim, where each plugin gets its name passed to it via loadfile(init.vim)(plugin_name). Plugin names are unique, right? That could be used then to namespace functions properly.

But vim also provides SID and Plug for things like this, we should investigate if this can be used.

@hkupty
Copy link
Contributor

hkupty commented Jul 20, 2018

guess an idea would by namespacing-by-neovim, where each plugin gets its name passed to it via loadfile(init.vim)(plugin_name).

I have no clue here. How do we retrieve that from lua side, so we can name things accordingly?

Also, we still have the "how do I name this function?" problem. We can namespace it according to the plugin name, but we still need a function "name" or something similar.

What if we have something like this:

tycho.map{
  id = "my_namespaced_id",
  keys = "...",
  fn = function() ... end
}

So we don't have to register the functions but we still have an unique name?

This simplifies things a lot tbh

@KillTheMule
Copy link
Author

I have no clue here. How do we retrieve that from lua side, so we can name things accordingly?

The arguments to loadfile are simply available as ... inside init.lua. If the first argument is the addon name, you can simply run local ns = ... inside and have your unique name available as ns. We could then add a table vim.lua_api.plugins.ns or something and stick the function in there.

@hkupty
Copy link
Contributor

hkupty commented Jul 20, 2018

This has some rough edges that need to be sorted out though:

tycho.map{
  id = "my_namespaced_id",
  keys = "...",
  fn = function() ... end
}

tycho.command{
  name = "MyCommand",
  id = "my_namespaced_id",
  nargs = 0,
  fn = function() ... end,
}

What happens? Do we overwrite or discard if both uses the same id?
Can we ignore the fn key if id is already bound? should we ensure that id is unique?

@hkupty
Copy link
Contributor

hkupty commented Jul 20, 2018

you can simply run local ns = ...

Mind is blown

@KillTheMule
Copy link
Author

KillTheMule commented Jul 20, 2018

What happens? Do we overwrite or discard if both uses the same id?

I'm not sure I understand it fully. What's the role of id and keys exactly? I assume name refers to the {cmd} argument of :command.

Oh yeah, and one possibility, at least in my mind, is only allowing functions in fn that are exported with the module, by means of require(ns).fn ~= nil.

@hkupty
Copy link
Contributor

hkupty commented Jul 20, 2018

Ah! Now I got your point.

I was thinking that id should be the binding id name.

Now it makes sense. I'll try to sketch something like that.

@hkupty
Copy link
Contributor

hkupty commented Jul 20, 2018

Tested and this works:

tycho.map = function(obj)
  local command = "lua require('" .. obj.ns .. "')." .. obj.fn .. "("
  if obj.args ~= nil and #obj.args >= 0 then
    for _, i in ipairs(obj.args) do
      if type(i) == "string" then
        command = command .. ", '" .. i .. "'"
      else
        command = command .. ", " .. i
      end
    end
  end
  command = command .. ")"

  nvim.nvim_command("map " .. obj.keys .. " <Cmd>" .. command .. "<CR>")
end

Btw, I ended up using a single argument, so values can be provided as keywords:

tycho.map{
  ns = ...,
  fn = "myfn",
  args = {"arg", 123}, -- optional
  keys = "<leader>p"
}

This seems clearer than:

tycho.map(..., "myfn", "<leader>p", {"arg", 123})

The optional args keyword could be used if a function requires arguments:

myns.myfn = function(str, count)
...
end

@hkupty hkupty mentioned this issue Jul 20, 2018
@quark-zju
Copy link

quark-zju commented Jul 20, 2018

Coming from a new user who found nvim API not expressive enough for internal lua usecase (ex. no autocmd, nvim_subscribe only works for reomte plugin). This direction looks promising to me. I guess for autocmd to be implemented using the current API, some VimL glue layer is needed, which seems to be less ideal.

Is the plan to experiment with APIs here, and eventually add native lua APIs to nvim directly (for efficiency and avoid string-escaping issues)? Before I saw this thread, I was thinking about adding a callback style nvim_subscribe for internal usecase. Maybe that is one of the "missing APIs"?

@hkupty
Copy link
Contributor

hkupty commented Jul 20, 2018

@quark-zju absolutely! We're experimenting with the API to find out the most idiomatic way glue lua and neovim.

Subscribing to events is definitely an item on this list and I'm sure we will find more.

Patches are welcome and so are suggestions!

@bfredl
Copy link

bfredl commented Jul 20, 2018

if obj.args ~= nil and #obj.args >= 0 then
   for _, i in ipairs(obj.args) do
     if type(i) == "string" then
       command = command .. ", '" .. i .. "'"
     else
       command = command .. ", " .. i
     end
   end
 end

Lua already supports closures, I don't see much point of the glue layer "managing" already-known lua arguments to functions, the plugin could deal with this themselves with far simpler code than in the example above.

@hkupty
Copy link
Contributor

hkupty commented Jul 20, 2018

Since we're dropping the indirection layer and formalising conventions, I'm fine with either path, as long as we're consistent and that doesn't become a problem.

Allowing arguments to be supplied might impact less the client code, which would otherwise need to expose a no argument version of that same function so that one can be mapped.

Again, I'm fine with that, as long as we agree on that, not from the lua implementation perspective, but from the API optic.

@KillTheMule
Copy link
Author

KillTheMule commented Jul 20, 2018

Ok, I kinda went with my "normal" workflow and [ntegrated tycho into the neovim code base on a new neovim branch. I added a testplugin to test how a plugin writer would use this, and I added a test for the plugin.

I've also added the extmark PR since it contains nvim_create_namespace, which we're not yet using, but I think since this functionality is already usefull for another PR, we'd be well advised to look into using it. Imho repeated calls with the same string should result in the same int returned, but I'll leave that for now.

I'll want to see if we can use that to make the fn argument really a lua function instead of a string.

I'm thinking about requiring plugin writers to start off with

let this = vim.lua_api.new_plugin("pluginname")

where we return a read-only table that contains metadata for the plugin, i.e the name (which was passed), a unique namespace, and whatever. This function could also require all the api stuff, so it only gets loaded when a plugin needs it. With metatable shennigans, we could output a warning if someone tries to use the lua_api without this call. Haven't thought about the vimscript side, but that's somewhat secondary to me. Thoughts on this?

@bfredl
Copy link

bfredl commented Jul 20, 2018

Allowing arguments to be supplied might impact less the client code, which would otherwise need to expose a no argument version of that same function so that one can be mapped.

I find this argument quite weird, as if plugin authors or users would just write functions at random and then later randomly decide some functions should be exposed as mappings with some specific arguments. Won't a real-world plugin (or a init.lua) being written with user functionality in mind and thus will have a layer functions that directly correspond to commands, autocmds and mappings, with the arguments that are natural for those?

That notwithstanding, even if we allow an args arguments for convenience we can construct a closure in the wrapper layer, instead of trying to "serialize" lua values as a vimL string which will bound to fail with any non-trival plugin code.

@hkupty
Copy link
Contributor

hkupty commented Jul 20, 2018

as if plugin authors or users would just write functions at random and then later randomly decide some functions should be exposed as mappings with some specific arguments.

Actually, this is quite the contrary... I don't want to make any assumptions or impose any rule to client code. I see the value on what you're proposing and we could go either way.

in the wrapper layer

We probably don't need a wrapper layer as @KillTheMule stated above. In that case, I prefer not having one only for convenience wrappers.

@bfredl
Copy link

bfredl commented Jul 20, 2018

I don't want to make any assumptions or impose any rule to client code.

But my argument does not. Any lua code could create a closure on the fly to match an existing function to a callback signature, including adding args, removing args, or even changing the order of args, etc. Having an args convenience argument to add args to a call communicates to me the assumption that this is expected to be a common operation. (which indeed can be true, but then would expect an argument for that over status quo)

@hkupty
Copy link
Contributor

hkupty commented Jul 20, 2018

@bfredl I don't think we're on the same page and, tbh, I see no value defending further an option that doesn't have strong selling points. I said we could go either way as long as it makes sense to everyone.

If args complicate more than help, then we drop it for simplicity and move forward.

@KillTheMule, regarding your comment, since all plugins are modules, I think it's safe to assume they'll return a table. We could prepare that table so it contains this kind of metadata:

let plugin = vim.lua_api.new_plugin("pluginname")

plugin.myfn = function()
...
end

return plugin

@KillTheMule
Copy link
Author

KillTheMule commented Jul 20, 2018

I've put up something in this commit. I like this API, look at the testplugin. We can even relax the condition that the plugin exports the functions it maps, it's not needed. The namespace is also totally transparent now, but if a plugin needs it, it's there (say, for the extmark stuff). Also, a plugin doesnt need to call new_plugin, but if it wants to use the mapping stuff, it can't get that without the return value from new_plugin, because thismap is local to tycho.

@hkupty
Copy link
Contributor

hkupty commented Jul 20, 2018

Then we're back to the indirection layer on tycho.. 😅

It is good though. I like the direction this is heading.

@bfredl
Copy link

bfredl commented Jul 21, 2018

. I said we could go either way as long as it makes sense to everyone.

@hkupty Yeah you're right, sorry if my tone was a bit bad.

The only thing I would bikeshed over @KillTheMule proposal is to put the function arg last, as I think it looks better with inline closures (similar to busted):

ns:map("<F2>", function()
  doX("arg")
  doY()
end)

@hkupty
Copy link
Contributor

hkupty commented Jul 21, 2018

No worries. I tend to be picky and push my arguments over. I don't want that to happen, that's why I cut it there.

It is important to me that we reach a point where its so friendly and fluid as if it was a full replacement for viml.

This can (and probably will) dictate how plugin authors and users will migrate their stuff to lua.

@KillTheMule
Copy link
Author

I've streamlined things a bit and changed to a style that more suits my brain, hope @hkupty doesn't mind :) This als removed the need for utils, which makes the whole thing easier to understand imho.

As far as features go, I'd like to add an optional array to plugin:map where you can put in any of the optional arguments, like { ["buffer"] = true, ["silent"] = true }. Another feature would be a plugin:unmap function. Note that I've disallowed mapping the same key combo twice, because I'm using the Keys as keys (for the array). I think this is sensible, or am I missing a use-case here?

What else do we need for map functionality? Feels like this is a good first spot I guess. Anybody wants to suggest some tests?

@KillTheMule
Copy link
Author

KillTheMule commented Jul 21, 2018

I've hit an API snag while trying to implement optional arguments. Up to now, we can map using

plugin:map("<", stuff2)

Note that the keys argument is a string, and used as such. So in theory we could just let people prepend things like <buffer> themselves. As a consequence of a workaround to be able to use special key designators like <F2>, this doesn't really work, because <buffer> becomes <lt>buffer>. Doing this stringly seems wrong anyways. Should we parse the key argument to avoid such things? Then again, it's hard to keep people from passing in garbage anyways...

Now, how do we do optional arguments? We could make map of signature map(keys, fn, args) where the last is the optional dictionary of optional arguments, but this kinda breaks the idea of "fn is the last arg". Then again, a signature like map(keys, args, fn) makes it non-optional really, so people have to pass nil or {} in most use cases, which is kind of annoying.

I've figured out a different approach: fun with metatables. You can see how it's used in the test. You could keep in piling up optional arguments (althoug we'd surely be able to make each of them just unique for the final call), and they are used and cleared in the final map call.

Does that look like a good API? I kind of like it because for most mappings (the ones without an optional argument) it avoids any overhead, while still enabling the use somewhat programmatically. We could even throw an error if someone tries to put in an optional argument we don't like...

Thoughts?

(e) If the metatable approach seems right, it would be better imho to not clear the optional arguments on call, but keep them until someone calls, say plugin:map.clear_args() or such.

@bfredl
Copy link

bfredl commented Jul 21, 2018

an alternative could be to both support map(lhs, fn), and map{lhs=lhs,silent=true,fn=function() .. end}

Keeping options implicit in a metatable sounds overly stateful, for stuff like <buffer> I would rather imagine a busted-like approach which feels more like a context manager, which gives an explicit end which can't be forgotten:

ns:for_filetype("python", function(ft)
 ft:map("<f2>", run_python)
 ft:map("<f3>", complete_python)
end)

(not sure exactly of the semantics, the above would apply <buffer> mapping for python buffers, or maybe we could return an object to apply the group on demand to a buffer).

@KillTheMule
Copy link
Author

I've removed the state from the metatable approach, although the idea of map taking a table argument as well sounds ok, too.

The "busted-like" approach is too advanced for a first shot imho, although that's the kind of facility we'd ultimately provide, I think.

@bfredl
Copy link

bfredl commented Jul 22, 2018

The "busted-like" approach is too advanced for a first shot imho,

Maybe, but I don't see how it would be any more complicated than the prefix approach, if we already are considering generating nested metatables on the fly, I definitely don't think dealing with a few closures somehow is "too advanced" in comparison. (I'll be happy to produce some prototype code, if that is the thing). Buf if initial simplicity actually is a goal, I would just start with map{lhs="key", silent=true, buffer=true, fn=...}

@KillTheMule
Copy link
Author

Maybe, but I don't see how it would be any more complicated

That is because I didn't express myself properly, early morning without coffee and all that. What I actually meant is that example you gave, where you already provide a way to make a map by filetype, which probably includes an autocommand for future buffers or something. That's a bit more than the "first shot" I imagine. If it simply falls out, sure :)

As for some prototype code, that would be great, so we can compare and see what we get.

@bfredl
Copy link

bfredl commented Jul 22, 2018

Yes, but I think we want to include autocommands initially as it is essential for plugins, and ft support would just a thin wrapper above that. Even without it you could write

ns:autocmd("FileType", "python", function()
 ns:map{"<f2>", buffer=true, fn=run_python}
 ns:map{"<f3>", buffer=true, fn=complete_python}
end)

I have some code for this (with mode support, but no namespaces yet), Am traveling today, but could finish and show it later.

@hkupty
Copy link
Contributor

hkupty commented Jul 26, 2018

Sorry for taking long to answer.

@KillTheMule I'm totally fine with changing what I did. Feel free to experiment.

I'd like to see more of this snippet @bfredl posted. It seems like an interesting path.

To be honest, I like this ns:map{...} structure better than the way it was before. Better avoid the magical stuff and straightforward.

I think adding the metatable can lead to issues, as things will be dependent on a state:

local plugin = ...
local map_buffer = plugin.map.buffer

map_buffer("<space>1", fn1)

-- Some code

map_buffer = plugin.map -- for some reason, whatever.

-- Some more code

map_buffer("<space>2", fn2) -- this will not be obvious.

You can obviously commit the same mistakes with the ns:map{...} approach, but I think it's a bit more difficult. I prefer invoking the zen of python and try to follow something similar (specially the first lines):

Explicit is better than implicit.

Flat is better than nested.

@KillTheMule
Copy link
Author

Ok, I want to push this a bit, I reverted the metatable-commits and went for a simple solution. I tried to catch as many possible errors as I could, since map isn't really performance critical to not be able to afford that.

The only thing I'm thinking about is that people might sneak strange stuff in the keys argument, likes using keys = "<F2> <Plug>", and I don't know what would happen. Should I check for unescaped spaces in keys, or is that overdoing it? Anything else this should do? I guess the match stuff should get it's own file, though...

Any other ideas? Next would be "command", I guess.

@KillTheMule
Copy link
Author

KillTheMule commented Aug 6, 2018

I've restructured things an provided docs. I'll join the namespace discussion here. "Command" ahead :)

(e) I also added unmap. I've restricted it to unmapping keys that were bound from the plugin, just because it made sense structurally. You can still use command(":unmap xyz") if you want to break out of that.

@hkupty
Copy link
Contributor

hkupty commented Aug 9, 2018

I've been on a rush for the last couple of weeks, but that is really good.

Thanks for pushing this forward, @KillTheMule!

@KillTheMule
Copy link
Author

NP :)

I've thought about it some more, and while I don't think we need to support "", we should provide mapping possibilities not only for Cmd-mappings, but also the other mode-mappings as well as nonrecursive ones (which are probably the standard anyways). Quite a can of worms, but I think it needs to be done.

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

No branches or pull requests

4 participants