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

[RDY] Refactor/enhance job api #2247

Merged
merged 6 commits into from Mar 30, 2015

Conversation

Projects
None yet
@tarruda
Member

tarruda commented Mar 26, 2015

This is the refactor I mentioned in #2076. It aims to simplify and optimize the job API, removing the JobActivity/v:job_data and calling functions instead of autocmds.

It is also necessary to avoid deferred events and needless preprocessing of job data when a terminal is spawned(Currently every chunk of data received from the job is split into a list of lines with NULs replaced by linefeeds, even if no job handlers are registered)

Close #1838

@marvim marvim added the WIP label Mar 26, 2015

@rainerborene

This comment has been minimized.

Show comment
Hide comment
@rainerborene

rainerborene Mar 26, 2015

Contributor

@tarruda any reason to use on prefix with underscores? why not follow the HTML spec on that matter?

Contributor

rainerborene commented Mar 26, 2015

@tarruda any reason to use on prefix with underscores? why not follow the HTML spec on that matter?

@tarruda tarruda referenced this pull request Mar 26, 2015

Closed

Neovim compatibility #137

@starcraftman starcraftman referenced this pull request Mar 26, 2015

Merged

New Job API #198

@bfredl

This comment has been minimized.

Show comment
Hide comment
@bfredl

bfredl Mar 26, 2015

Member

I don't remember if it was discussed before or not, but have you considered using/supporting the already existing Dictionary-Function pattern, i e when a function is defined directly inside a dict, it can access that dict through an implicit self argument:

let mydict = {'data': [0, 1, 2, 3]}
function mydict.len()
   return len(self.data)
endfunction
Member

bfredl commented Mar 26, 2015

I don't remember if it was discussed before or not, but have you considered using/supporting the already existing Dictionary-Function pattern, i e when a function is defined directly inside a dict, it can access that dict through an implicit self argument:

let mydict = {'data': [0, 1, 2, 3]}
function mydict.len()
   return len(self.data)
endfunction
Show outdated Hide outdated runtime/doc/eval.txt Outdated
Show outdated Hide outdated runtime/doc/eval.txt Outdated
Show outdated Hide outdated runtime/doc/eval.txt Outdated
Show outdated Hide outdated runtime/doc/job_control.txt Outdated
Show outdated Hide outdated runtime/doc/eval.txt Outdated
Show outdated Hide outdated runtime/doc/eval.txt Outdated
Show outdated Hide outdated runtime/doc/job_control.txt Outdated
Show outdated Hide outdated runtime/doc/job_control.txt Outdated
char **argv = xcalloc(argc + 1, sizeof(char *));
for (listitem_T *arg = args->lv_first; arg != NULL; arg = arg->li_next) {
argv[i++] = xstrdup((char *)arg->li_tv.vval.v_string);
}

This comment has been minimized.

@oni-link

oni-link Mar 26, 2015

Contributor

argv[i] = NULL; is missing.
Ah, memory allocated with xcalloc().

@oni-link

oni-link Mar 26, 2015

Contributor

argv[i] = NULL; is missing.
Ah, memory allocated with xcalloc().

This comment has been minimized.

@splinterofchaos

splinterofchaos Mar 29, 2015

Member

v_string may be NULL in some circumstances to represent an empty string. get_tv_string() would return an empty string in this case, and also allow the implicit conversion of integers to strings. Same comment for the os_can_exe() call above.

ref: #1471 for an instance where this may segfault.

@splinterofchaos

splinterofchaos Mar 29, 2015

Member

v_string may be NULL in some circumstances to represent an empty string. get_tv_string() would return an empty string in this case, and also allow the implicit conversion of integers to strings. Same comment for the os_can_exe() call above.

ref: #1471 for an instance where this may segfault.

Show outdated Hide outdated src/nvim/eval.c Outdated
@tarruda

This comment has been minimized.

Show comment
Hide comment
@tarruda

tarruda Mar 27, 2015

Member

I don't remember if it was discussed before or not, but have you considered using/supporting the already existing Dictionary-Function pattern, i e when a function is defined directly inside a dict, it can access that dict through an implicit self argument:

Is there a difference from the "user" option? Its already possible to do this:

function s:Handler(id, data, user)
endfunction

call jobstart([..], {'on_stdout': 's:Handler', 'user': {}})

edit: Plus the "user" option allow passing strings, numbers or lists(Is it possible for self to be a non-dict?).

Member

tarruda commented Mar 27, 2015

I don't remember if it was discussed before or not, but have you considered using/supporting the already existing Dictionary-Function pattern, i e when a function is defined directly inside a dict, it can access that dict through an implicit self argument:

Is there a difference from the "user" option? Its already possible to do this:

function s:Handler(id, data, user)
endfunction

call jobstart([..], {'on_stdout': 's:Handler', 'user': {}})

edit: Plus the "user" option allow passing strings, numbers or lists(Is it possible for self to be a non-dict?).

@bfredl

This comment has been minimized.

Show comment
Hide comment
@bfredl

bfredl Mar 27, 2015

Member

Nothing except consistency with existing vimscript functionality. Not that
I know if theyre actaully used to any extent, but if we're settling for
funcrefs within a dict as callback pattern, there should be an explicit
desicion to either support or deprecate dict-functions.
Den 27 mar 2015 01:01 skrev "Thiago de Arruda" notifications@github.com:

I don't remember if it was discussed before or not, but have you
considered using/supporting the already existing Dictionary-Function
pattern, i e when a function is defined directly inside a dict, it can
access that dict through an implicit self argument:

Is there a difference from the "user" option? Its already possible to do
this:

function s:Handler(id, data, user)endfunction

call jobstart([..], {'on_stdout': 's:Handler', 'user': {}})


Reply to this email directly or view it on GitHub
#2247 (comment).

Member

bfredl commented Mar 27, 2015

Nothing except consistency with existing vimscript functionality. Not that
I know if theyre actaully used to any extent, but if we're settling for
funcrefs within a dict as callback pattern, there should be an explicit
desicion to either support or deprecate dict-functions.
Den 27 mar 2015 01:01 skrev "Thiago de Arruda" notifications@github.com:

I don't remember if it was discussed before or not, but have you
considered using/supporting the already existing Dictionary-Function
pattern, i e when a function is defined directly inside a dict, it can
access that dict through an implicit self argument:

Is there a difference from the "user" option? Its already possible to do
this:

function s:Handler(id, data, user)endfunction

call jobstart([..], {'on_stdout': 's:Handler', 'user': {}})


Reply to this email directly or view it on GitHub
#2247 (comment).

@tarruda

This comment has been minimized.

Show comment
Hide comment
@tarruda

tarruda Mar 27, 2015

Member

Nothing except consistency with existing vimscript functionality. Not that
I know if theyre actaully used to any extent, but if we're settling for
funcrefs within a dict as callback pattern, there should be an explicit
desicion to either support or deprecate dict-functions.

I could change the "user" option to "self" and pass it as a implicit self argument, but nothing would stop the user from doing this:

function s:Handler(id, data)
  self.id = 1  " self is available
endfunction

call jobstart([..], {'on_stdout': 's:Handler', 'self': {id: 0}})

Even though s:Handler is not a dictionary function. Also, I don't think there's a way avoid having to pass the "self" function despite using a dictionary function as callback:

let dict = {id: 1}
function dict.handler(id, data)
  echo self.id  " self.id == 0
endfunction

call jobstart([..], {'on_stdout': dict.handler, 'self': {id: 0}})

That is, self will always be what is passed to jobstart, not the dict where the function was defined(unless the vimscript engine "binds" dict.handler to dict, but I don't think thats the case)

Member

tarruda commented Mar 27, 2015

Nothing except consistency with existing vimscript functionality. Not that
I know if theyre actaully used to any extent, but if we're settling for
funcrefs within a dict as callback pattern, there should be an explicit
desicion to either support or deprecate dict-functions.

I could change the "user" option to "self" and pass it as a implicit self argument, but nothing would stop the user from doing this:

function s:Handler(id, data)
  self.id = 1  " self is available
endfunction

call jobstart([..], {'on_stdout': 's:Handler', 'self': {id: 0}})

Even though s:Handler is not a dictionary function. Also, I don't think there's a way avoid having to pass the "self" function despite using a dictionary function as callback:

let dict = {id: 1}
function dict.handler(id, data)
  echo self.id  " self.id == 0
endfunction

call jobstart([..], {'on_stdout': dict.handler, 'self': {id: 0}})

That is, self will always be what is passed to jobstart, not the dict where the function was defined(unless the vimscript engine "binds" dict.handler to dict, but I don't think thats the case)

@pgdouyon

This comment has been minimized.

Show comment
Hide comment
@pgdouyon

pgdouyon Mar 27, 2015

Sorry if I'm misunderstanding something and this went completely over my head, but I understood @bfredl's suggestion as something like this:

let dict = {'on_stdout': function("s:Handler"), 'id': 0}
function! s:Handler() dict
    echo self.id
endfunction

call jobstart([..], dict)

Where self is the dictionary passed to jobstart, not a key in that dictionary, so that handler parameters can be stored directly in the dictionary and not passed through the "user" entry. Personally, I think calling the handlers as dictionary functions is a bit more elegant, but the "user" key is valuable in that it provides a separate namespace for parameters and avoids collisions with job-control options.

pgdouyon commented Mar 27, 2015

Sorry if I'm misunderstanding something and this went completely over my head, but I understood @bfredl's suggestion as something like this:

let dict = {'on_stdout': function("s:Handler"), 'id': 0}
function! s:Handler() dict
    echo self.id
endfunction

call jobstart([..], dict)

Where self is the dictionary passed to jobstart, not a key in that dictionary, so that handler parameters can be stored directly in the dictionary and not passed through the "user" entry. Personally, I think calling the handlers as dictionary functions is a bit more elegant, but the "user" key is valuable in that it provides a separate namespace for parameters and avoids collisions with job-control options.

@kopischke

This comment has been minimized.

Show comment
Hide comment
@kopischke

kopischke Mar 27, 2015

@bfredl

I know if theyre actaully used to any extent, but if we're settling for funcrefs within a dict as callback pattern, there should be an explicit desicion to either support or deprecate dict-functions.

I use dictionary functions all the time in my plugins – they are one of the rare truly high level Vim script features, allowing OOP type encapsulation of behaviour.

@tarruda

self will always be what is passed to jobstart, not the dict where the function was defined (unless the vimscript engine "binds" dict.handler to dict, but I don't think thats the case)

It doesn't – the behaviour you describe is consistent with what call() does: it binds its optional third argument (which needs to be a Dictionary) to self for the purpose of a dictionary Funcref passed as its first argument. It’s not a perfect solution by a far cry (what in Vim script is?), but I am not sure adding yet another set of Funcref semantics is going to help with that :).

kopischke commented Mar 27, 2015

@bfredl

I know if theyre actaully used to any extent, but if we're settling for funcrefs within a dict as callback pattern, there should be an explicit desicion to either support or deprecate dict-functions.

I use dictionary functions all the time in my plugins – they are one of the rare truly high level Vim script features, allowing OOP type encapsulation of behaviour.

@tarruda

self will always be what is passed to jobstart, not the dict where the function was defined (unless the vimscript engine "binds" dict.handler to dict, but I don't think thats the case)

It doesn't – the behaviour you describe is consistent with what call() does: it binds its optional third argument (which needs to be a Dictionary) to self for the purpose of a dictionary Funcref passed as its first argument. It’s not a perfect solution by a far cry (what in Vim script is?), but I am not sure adding yet another set of Funcref semantics is going to help with that :).

@bfredl

This comment has been minimized.

Show comment
Hide comment
@bfredl

bfredl Mar 27, 2015

Member

@tarruda I didn't mean introduce yet another semantics for funcref passing, just supporting the existing semantics for calling a function inside a dict, i e if jobstart "was a vimscript function" the following would be expected to work

let handlers = {'myid':'0'}
function handlers.on_stdout(id, data)
    "here self.myid is available
end

function handlers.on_exit(id, data)
    "also here
end

call jobstart(args, handlers)

" separate instance
call jobstart(args, extend(handlers, {'myid':1}))

but as @pgdouyon pointed out, there would be a namespace conflict with the options here, so if this pattern was "actively" supported, there would probably be a separate "handlers" argument/option.

Member

bfredl commented Mar 27, 2015

@tarruda I didn't mean introduce yet another semantics for funcref passing, just supporting the existing semantics for calling a function inside a dict, i e if jobstart "was a vimscript function" the following would be expected to work

let handlers = {'myid':'0'}
function handlers.on_stdout(id, data)
    "here self.myid is available
end

function handlers.on_exit(id, data)
    "also here
end

call jobstart(args, handlers)

" separate instance
call jobstart(args, extend(handlers, {'myid':1}))

but as @pgdouyon pointed out, there would be a namespace conflict with the options here, so if this pattern was "actively" supported, there would probably be a separate "handlers" argument/option.

@tarruda

This comment has been minimized.

Show comment
Hide comment
@tarruda

tarruda Mar 27, 2015

Member

I've refactored to be more friendly with the dictionary pattern. Even with the possible namespace clashes, I think its simpler to reuse the job options object as "self", its not like the user will run out of keys 😄

Member

tarruda commented Mar 27, 2015

I've refactored to be more friendly with the dictionary pattern. Even with the possible namespace clashes, I think its simpler to reuse the job options object as "self", its not like the user will run out of keys 😄

Show outdated Hide outdated runtime/doc/job_control.txt Outdated

@tarruda tarruda changed the title from [WIP] Refactor/enhance job api to [RDY] Refactor/enhance job api Mar 27, 2015

@tarruda

This comment has been minimized.

Show comment
Hide comment
@tarruda

tarruda Mar 27, 2015

Member

Added some basic documentation for the terminal emulator. Didn't elaborate much because some things will be changing in the next days as we get feedback from users(#2171, #2257 ...)

Member

tarruda commented Mar 27, 2015

Added some basic documentation for the terminal emulator. Didn't elaborate much because some things will be changing in the next days as we get feedback from users(#2171, #2257 ...)

@marvim marvim added RDY and removed WIP labels Mar 27, 2015

@tarruda tarruda changed the title from [RDY] Refactor/enhance job api to [WIP] Refactor/enhance job api Mar 27, 2015

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Mar 28, 2015

@tarruda I sent you a PR with some improvements (no elaboration as you said, just low hanging fruit).

ghost commented Mar 28, 2015

@tarruda I sent you a PR with some improvements (no elaboration as you said, just low hanging fruit).

@tarruda

This comment has been minimized.

Show comment
Hide comment
@tarruda

tarruda Mar 28, 2015

Member

@tarruda I sent you a PR with some improvements (no elaboration as you said, just low hanging fruit).

👍 applied your fixes, thanks

Member

tarruda commented Mar 28, 2015

@tarruda I sent you a PR with some improvements (no elaboration as you said, just low hanging fruit).

👍 applied your fixes, thanks

@tarruda

This comment has been minimized.

Show comment
Hide comment
@tarruda

tarruda Mar 28, 2015

Member

Implemented the jobwait function which should allow @junegunn to close junegunn/vim-plug#104

Also modified jobsend so that the second argument is optional. If omitted, the job stdin is closed. @bfredl can you verify if this meets #1838 requirements?

Member

tarruda commented Mar 28, 2015

Implemented the jobwait function which should allow @junegunn to close junegunn/vim-plug#104

Also modified jobsend so that the second argument is optional. If omitted, the job stdin is closed. @bfredl can you verify if this meets #1838 requirements?

@bfredl

This comment has been minimized.

Show comment
Hide comment
@bfredl

bfredl Mar 28, 2015

Member

@tarruda It does what's required, but enables to crash nvim:

let j = jobopen(["xsel", "-n", "-i"])
call jobsend(j, ["some text"])
call jobsend(j)
call jobsend(j, ["this crashes nvim"])

Also, would it hurt to have separate jobclose ? ( jobsend(j, data); jobsend(j) isn't really self-explanatory)

Member

bfredl commented Mar 28, 2015

@tarruda It does what's required, but enables to crash nvim:

let j = jobopen(["xsel", "-n", "-i"])
call jobsend(j, ["some text"])
call jobsend(j)
call jobsend(j, ["this crashes nvim"])

Also, would it hurt to have separate jobclose ? ( jobsend(j, data); jobsend(j) isn't really self-explanatory)

@tarruda

This comment has been minimized.

Show comment
Hide comment
@tarruda

tarruda Mar 29, 2015

Member

@bfredl Implemented your jobclose suggestion

Member

tarruda commented Mar 29, 2015

@bfredl Implemented your jobclose suggestion

@ZyX-I

This comment has been minimized.

Show comment
Hide comment
@ZyX-I

ZyX-I Mar 29, 2015

Contributor

I could change the "user" option to "self" and pass it as a implicit self argument, but nothing would stop the user from doing this:

function s:Handler(id, data)
  self.id = 1  " self is available
endfunction

call jobstart([..], {'on_stdout': 's:Handler', 'self': {id: 0}})

If you did not alter code that calls user functions then self is not passed to non-dictionary function even if it was given explicitly by call. Try the following:

function NonDict()
    echo l:
endfunction
function Dict() dict
    echo l:
endfunction
call call('NonDict', [], {}) " echoes {}
call call('Dict', [], {}) " echoes {'self': {}}
Contributor

ZyX-I commented Mar 29, 2015

I could change the "user" option to "self" and pass it as a implicit self argument, but nothing would stop the user from doing this:

function s:Handler(id, data)
  self.id = 1  " self is available
endfunction

call jobstart([..], {'on_stdout': 's:Handler', 'self': {id: 0}})

If you did not alter code that calls user functions then self is not passed to non-dictionary function even if it was given explicitly by call. Try the following:

function NonDict()
    echo l:
endfunction
function Dict() dict
    echo l:
endfunction
call call('NonDict', [], {}) " echoes {}
call call('Dict', [], {}) " echoes {'self': {}}

tarruda added some commits Mar 26, 2015

eval: Refactor `call_func` and `func_unref`
- Make it possible to call or unref ufunc_T pointers directly.
- Keep refcount of named functions, and stop them from being deleted if the
  refcount is greater than 1.
eval: Refactor vimscript job control API
- Remove JobActivity autocmd and v:job_data variable
- Simplify `jobstart` to receive:
  - An argument vector
  - An optional dictionary which may contain any of the current `jobstart`
    options plus `on_stdout`, `on_stderr` and `on_exit` callbacks.
- Refactor and add more job tests
- Update documentation
eval: Improve validation of ids passed to job functions
Use the `is_user_job` to ensure that the job was started by `jobstart` or
`termopen`.
doc: Begin terminal emulator documentation
With some spacing/indentation fixes.

Helped by: @Pyrohh, @kopischke

@tarruda tarruda changed the title from [WIP] Refactor/enhance job api to [RDY] Refactor/enhance job api Mar 29, 2015

@tarruda tarruda merged commit b94f290 into neovim:master Mar 30, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@jszakmeister jszakmeister removed the WIP label Mar 30, 2015

tarruda added a commit that referenced this pull request Mar 30, 2015

@tarruda tarruda referenced this pull request Mar 30, 2015

Closed

job control enhancements #1869

@bfredl

This comment has been minimized.

Show comment
Hide comment
@bfredl

bfredl Mar 30, 2015

Member

Just noted a small discrepancy between jobstart() and termopen(), while the former accepts an argv-list only, the later only accepts a shell command string. Any particular reason for this or wouldn't it make more sense for both functions to accept both formats?

Member

bfredl commented Mar 30, 2015

Just noted a small discrepancy between jobstart() and termopen(), while the former accepts an argv-list only, the later only accepts a shell command string. Any particular reason for this or wouldn't it make more sense for both functions to accept both formats?

@tarruda

This comment has been minimized.

Show comment
Hide comment
@tarruda

tarruda Mar 31, 2015

Member

Just noted a small discrepancy between jobstart() and termopen(), while the former accepts an argv-list only, the later only accepts a shell command string. Any particular reason for this or wouldn't it make more sense for both functions to accept both formats?

No reason, it's simply a consequence of how termopen() was extracted from :terminal. For :terminal I think its important to continue using the shell so pipes and other shell features can be used naturally in nvim command line, but termopen should probably be refactored to receive argv like jobstart.

Member

tarruda commented Mar 31, 2015

Just noted a small discrepancy between jobstart() and termopen(), while the former accepts an argv-list only, the later only accepts a shell command string. Any particular reason for this or wouldn't it make more sense for both functions to accept both formats?

No reason, it's simply a consequence of how termopen() was extracted from :terminal. For :terminal I think its important to continue using the shell so pipes and other shell features can be used naturally in nvim command line, but termopen should probably be refactored to receive argv like jobstart.

@justinmk

This comment has been minimized.

Show comment
Hide comment
@justinmk

justinmk Mar 31, 2015

Member

any reason to use on prefix with underscores? why not follow the HTML spec on that matter?

@rainerborene I owe you a retraction: I realize now you were talking about the vimscript "on_stdin", "on_stdout", "on_stderr" keys, not the C source code. Yes, I agree we should avoid underscores in vimscript identifiers, if possible.

Member

justinmk commented Mar 31, 2015

any reason to use on prefix with underscores? why not follow the HTML spec on that matter?

@rainerborene I owe you a retraction: I realize now you were talking about the vimscript "on_stdin", "on_stdout", "on_stderr" keys, not the C source code. Yes, I agree we should avoid underscores in vimscript identifiers, if possible.

@justinmk

This comment has been minimized.

Show comment
Hide comment
@justinmk

justinmk Mar 31, 2015

Member

I think its important to continue using the shell so pipes and other shell features can be used naturally in nvim command line, but termopen should probably be refactored to receive argv like jobstart.

👍

Member

justinmk commented Mar 31, 2015

I think its important to continue using the shell so pipes and other shell features can be used naturally in nvim command line, but termopen should probably be refactored to receive argv like jobstart.

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment