Adding lua function to use result of RunBackgroundShell

[Neko-Box-Coder]

Currently, RunBackgroundShell() returns a function and an error. I don't think there's a way to run that function asynchronously from lua plugin.

Adding a helper lua function for that.

Although there's an additional error to the return of the returned function from RunBackgroundShell(), this has no impact the lua plugins that are currently using it since it first return var is the same. I have tested this with a lua plugin.

[niten94]

I do not know what use cases does RunBackgroundShell() have since it's somewhat similar with RunCommand(), but JobStart() can be used like this to mostly achieve the purpose of the function added:

local micro = import("micro")
local shell = import("micro/shell")

local job

local function onexit(output, data)
    local state = job.ProcessState
    -- ProcessState is set if process (sh if using JobStart) exits
    -- onExit is called even if an error occurs with something like I/O within
    -- Go, but it is probably rare
    --if state == nil then return end

    micro.TermMessage(state:Success() and output or state:String())
end

function init()
    job = shell.JobStart("kill $$", nil, nil, onexit)
end

I think it is better to instead change the function so that it returns an error when occuring on process startup, and passes the error that cmd.Wait returns to onExit.

[Neko-Box-Coder]

The main thing I wanted was to able to do something like RunCommand() but doesn't block the main thread, for example something like a curl command which might take a couple seconds.

Yeah it's true that I can do JobStart() (Although only Linux since it uses sh -c which this PR also fix) and I actually didn't know you can get the exit state with job.ProcessState.

But even then you still need to keep track of the command execution by storing it in global (probably in a map or some sort?)

If this PR gets merged, what you need to do instead it's just

local function onexit(output, err)
    -- Handle err here...
    micro.TermMessage(output)
end

function init()
    local retFunc, err = shell.RunBackgroundShell("curl whatever")
    -- Handle err...
    shell.LaunchBackgroundShellFunc(retFunc, onexit)
end

where you can handle the error directly in the callback as param, not to mention it has a simpler interface than JobStart(...)

In my mind, the Job* functions make sense for long-running tasks like LSP but overkill and painful to manage for a command that takes a few seconds to complete.

[niten94]

But even then you still need to keep track of the command execution by storing it in global (probably in a map or some sort?)

Yes, that is right. Tables in userargs are copied since they are converted to an array or map in Go, so passing a table containing the job like this cannot be done:

local t = { i = 1 }
-- in onexit, userargs[1] is { i = 1 } with job not set
t.job = shell.JobStart("sleep 1", nil, nil, onexit, t)
t.i = 2

There are workarounds, so I don't think changing the function to pass Lua values without conversion needs to be prioritized. Arrays converted using gopher-luar can be iterated by writing array() unlike ipairs(t) used with tables, so changing this has a low chance to break existing plugins.

where you can handle the error directly in the callback as param, not to mention it has a simpler interface than JobStart(...)

I agree that the interface is simpler, especially since no objects have to be created in plugins to check errors. However, the interface seems a bit odd since the names are similar even though RunBackgroundShell and the function returned doesn't run programs asynchronously.

A function that's like a simplified version of JobStart, takes command as argument and immediately runs it asynchronously, while it returns errors or passes them to the callback may be better.

In my mind, the Job* functions make sense for long-running tasks like LSP but overkill and painful to manage for a command that takes a few seconds to complete.

I have no experience with adding APIs so I'll leave this to maintainers or others to decide and discuss.

[Neko-Box-Coder]

But even then you still need to keep track of the command execution by storing it in global (probably in a map or some sort?)

Yes, that is right. Tables in userargs are copied since they are converted to an array or map in Go, so passing a table containing the job like this cannot be done:

local t = { i = 1 }
-- in onexit, userargs[1] is { i = 1 } with job not set
t.job = shell.JobStart("sleep 1", nil, nil, onexit, t)
t.i = 2

I think you misunderstood what I meant. I meant you need to keep track of the job object returned by the JobStart(...) function for each call, if you need to know the return code / result of that command that is.

---

where you can handle the error directly in the callback as param, not to mention it has a simpler interface than JobStart(...)

I agree that the interface is simpler, especially since no objects have to be created in plugins to check errors. However, the interface seems a bit odd since the names are similar even though RunBackgroundShell and the function returned doesn't run programs asynchronously.

Yes, I totally agree that RunBackgroundShell doesn't actually run in the background is very confusing 😂

---

A function that's like a simplified version of JobStart, takes command as argument and immediately runs it asynchronously, while it returns errors or passes them to the callback may be better.

Yeah I would like to do that but I feel like that will be confusing as well since that will be doing what RunBackgroundShell is supposed to do.

Honestly though, I am happy to modify RunBackgroundShell to just run the command in the background if the maintainers are happy to break backward compatibility.

But I think at the minimum, we should at least merge the fix for JobStart() so that it works on Windows as well.

[niten94]

I think you misunderstood what I meant. I meant you need to keep track of the job object returned by the JobStart(...) function for each call, if you need to know the return code / result of that command that is.

Sorry, I meant to mention another approach on keeping track of the job object, that currently doesn't work even though it would be better than using something like a map in some cases.

But I think at the minimum, we should at least merge the fix for JobStart() so that it works on Windows as well.

I think it's better to submit this change in another pull request even if it's a lot smaller. I have done something similar in #3672 but I honestly have other pull requests where I haven't done such yet.