RFC: Add session option - #27
Conversation
|
This is an RFC for adding session support to vim-medieval. It's currently quite It currently has some checks in there for python (because that's what I use in This should probably be integrated into |
|
The main problem this is trying to address is where something is either costly Using require here would make the print block always take 5 seconds, whereas |
Adds a session option to codeblocks so previous results can be used. To check that a command is done running, we wait for an eof_token to appear. This allows us to not cut off commands with output early, but also doesn't rely on using sleep. **This commit does not support neovim**
Splits up retrieving the data from s:session_read_cb into separate helper functions for vim and nvim.
The use case makes sense to me. Though I think you should already be able to solve this today with no changes. For example, something like this: <!-- target: a -->
```python
import random
import time
time.sleep(5)
x = 42
print(f"x={x}")
```
<!-- name: a
```python
```
-->
<!-- require: a -->
```python
import random
print(random.randint(1, x))
```I understand this is a contrived example, but the basic idea is: in the first (expensive) block, write code to a temporary block ( Note that the |
|
That would probably solve a lot of cases. But I feel like it might run into Something like the following wouldn't work: It also seems rather cumbersome to have to add all the print statements one |
|
Apologies if you are on holiday or otherwise occupied. In which case please Just wanted to see if there were any more thoughts on this RFC. |
gpanders
left a comment
There was a problem hiding this comment.
As I said earlier the use case makes sense to me and I'm not opposed to the feature on principle but the PR needs some work.
Most of the comments I left are easily actionable, just a few small refactors. But there are some missing design points still, in particular how to map the "session" paradigm to each individual language without hard coding special cases into the source code.
| function! s:vim_cb(channel, msg) abort | ||
| for [k, s] in items(s:active_sessions) | ||
| if s.id == a:channel | ||
| call s:session_read(k, a:msg) | ||
| break | ||
| endif | ||
| endfor | ||
| endfunction | ||
|
|
||
| function! s:nvim_cb(job_id, data, event) abort | ||
| for [k, s] in items(s:active_sessions) | ||
| if s.id == a:job_id | ||
| call s:session_read(k, a:data) | ||
| break | ||
| endif | ||
| endfor | ||
| endfunction |
There was a problem hiding this comment.
These two functions are identical, let's consolidate (name it s:sessiondata since it's only used for sessions)
| call winrestview(view) | ||
| endfunction | ||
|
|
||
| function! s:session_read(key, lines) abort |
There was a problem hiding this comment.
Follow the conventions of the repo:
| function! s:session_read(key, lines) abort | |
| function! s:sessionread(key, lines) abort |
| break | ||
| endif | ||
| endfor | ||
| endfunction |
There was a problem hiding this comment.
Why do you have an exit callback for Nvim but not Vim?
Let's rename this s:sessionexit and re-use it for both Nvim and Vim (use a lambda if needed, look at the existing pattern in s:jobstart as a reference)
| endfor | ||
| endfunction | ||
|
|
||
| function! s:eval_session(lang, session_name, block, cb) abort |
There was a problem hiding this comment.
| function! s:eval_session(lang, session_name, block, cb) abort | |
| function! s:evalsession(lang, session_name, block, cb) abort |
| if a:lang ==# 'python' || a:lang ==# 'python3' | ||
| let cmd += ['-i', '-q'] | ||
| endif |
There was a problem hiding this comment.
We're not going to hard code languages in here (we have to do it for cmd to work around Windows wonkiness and even then I tried to avoid it). Let's find a way to make this work that is language agnostic and doesn't require special cases.
| if has('nvim') | ||
| let id = jobstart(cmd, { | ||
| \ 'on_stdout': function('s:nvim_cb'), | ||
| \ 'on_stderr': function('s:nvim_cb'), | ||
| \ 'on_exit': function('s:nvim_session_exit_cb'), | ||
| \ 'stdout_buffered': 0, | ||
| \ 'stderr_buffered': 0, | ||
| \ }) | ||
| if id <= 0 | ||
| return s:error('Failed to start job for ' . a:lang) | ||
| endif | ||
| let s:active_sessions[key] = { | ||
| \ 'id': id, | ||
| \ 'buffer': [], | ||
| \ 'token': '', | ||
| \ 'context': {}, | ||
| \ } | ||
| else | ||
| let job = job_start(l:cmd, { | ||
| \ 'out_cb': function('s:vim_cb'), | ||
| \ 'err_cb': function('s:vim_cb'), | ||
| \ 'mode': 'nl', | ||
| \ }) | ||
| let s:active_sessions[key] = { | ||
| \ 'id': job_getchannel(job), | ||
| \ 'job': job, | ||
| \ 'buffer': [], | ||
| \ 'token': '', | ||
| \ 'context': {}, | ||
| \ } | ||
| endif | ||
| endif |
There was a problem hiding this comment.
Use exists('*jobstart') instead of has('nvim') (look at s:jobstart as a reference) and extract this entire block into a function s:sessionstart.
| if a:lang =~# 'python' | ||
| let new_block += ['print("' . eof_token . '")'] | ||
| else | ||
| let new_block += ['echo "' . eof_token . '"'] | ||
| endif |
There was a problem hiding this comment.
Similar comment to earlier, we should not hard code language support here. And even then this is wrong, using echo for the else branch won't work.
| if has('nvim') | ||
| call chansend(session.id, new_block + ['']) | ||
| else | ||
| for line in new_block | ||
| call ch_sendraw(session.id, line . "\n") | ||
| endfor | ||
| endif |
There was a problem hiding this comment.
Follow the pattern of s:jobstart: make a function s:chansend that encapsulates the platform differences between Nvim and Vim and use exists('*chansend') instead of has('nvim')
Adds a session option to codeblocks so previous results can be used.
To check that a command is done running, we wait for an eof_token to appear. This allows us to not cut off commands with output early, but also doesn't rely on using sleep.