-
Notifications
You must be signed in to change notification settings - Fork 42
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
Change Syscall.CallAsync to return an explicit SyscallInvocation. #183
Conversation
Returns a SyscallInvocation dict from CallAsync calls that encapsulates the invocation's status and output. This can be extended in future changes to allow other types of interactions. It's also passed into the callbacks (backwards-compatible with the result_dict value), so if other data needs to be passed into callbacks in the future it can be attached to the invocation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Nice!
autoload/maktaba/syscall.vim
Outdated
" {callback} function will be called with the following arguments: | ||
" {callback}(result_dict), where result_dict contains stdout, stderr and status | ||
" (code). | ||
" If vim implementation does not support async execution (details below), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps "If the current Vim configuration does not...", since it's partly down to things like --servername
that aren't part of the implementation per se?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SG, changed that slightly to "if the current vim instance…" since it tends to be per-instance and "configuration" is a little too precise.
autoload/maktaba/syscall.vim
Outdated
if filereadable(a:stderr_file) | ||
let l:return_data.stderr = join(readfile(a:stderr_file), "\n") | ||
call delete(a:stderr_file) | ||
let l:invocation = s:pending_invocations[a:invocation_id] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it worth trapping an unknown invocation ID explicitly?
(Perhaps not, since you'll catch E716 on L455 anyway.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, and factored out a helper function to help keep the complexity of this function down.
" | ||
" Public variables: | ||
" * finished: 0 if invocation is pending, 1 if finished. The result variables | ||
" (status, stdout, stderr) will not exist if invocation is not finished. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: "if the invocation"?
" * stdout: the invocation's entire stdout string. | ||
" * stderr: the invocation's entire stderr string, if available. | ||
" | ||
" Note one Syscall invoked multiple times would produce multiple independent |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Note that"?
|
||
"" | ||
" @private | ||
" Create a @dict(SyscallInvocation). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: "Creates"? I think that's the prevailing style.
Thanks for the review! Addressed all comments. |
Returns a SyscallInvocation dict from CallAsync calls that encapsulates
the invocation's status and output. This can be extended in future
changes to allow other types of interactions. It's also passed into the
callbacks (backwards-compatible with the result_dict value), so if other
data needs to be passed into callbacks in the future it can be attached
to the invocation.