Added System.AsyncProcess.#838
Conversation
mikoto2000
force-pushed
the
add-async-process
branch
from
c2bdd67 to
da0b2af
Compare
mikoto2000
force-pushed
the
add-async-process
branch
from
0556520 to
e165c1d
Compare
|
draft 時は1コミットずつテストOKしないとだめだった、たぶん今は平気 だめだった……再度実行はしました |
| " v8.2.238 or earlier, job_stop() exit status is 0. | ||
| " See: https://github.com/vim/vim/commit/b3e195cca7b3201b188c1713b64012b1bef4f61f | ||
| if v:version == 802 && !has('patch238') || v:version < 802 | ||
| Assert True(g:exit_code == 0) | ||
| else | ||
| Assert True(g:exit_code != 0) | ||
| endif |
There was a problem hiding this comment.
えっと、パッチされたのは Windows だけな気がするので、Windowsとそれ以外では違う処理にしないとではないかと
|
別PRでサポートバージョンの変更をやってたのを忘れていました。 |
|
一応多めにレビュアー設定しましたが、+1名くらいレビューしてくださって、マージしていただければ、かなと。 |
tsuyoshicho
left a comment
tsuyoshicho
left a comment
There was a problem hiding this comment.
一度いいかなと思いましたが、再レビューしてコメントします。
https://github.com/tsuyoshicho/vital.vim/blob/master/autoload/vital/__vital__/Bitwise.vim
とかも参考になるかもです
すくなくとも、露出するIFの制御はしないとまずそうです
| if s:is_windows | ||
| " iconv() wrapper for safety. | ||
| function! s:iconv(expr, from, to) abort |
There was a problem hiding this comment.
-
処理重複
https://github.com/tsuyoshicho/vital.vim/blob/master/autoload/vital/__vital__/Process.vim
との処理重複がありそう
モジュールに depend して、s:iconv を使うでいいかもしれない -
呼び出しの正規化
処理に都度 if s:is_windows 書くのであれば、 s:_ensure_buffer_string を if s:is_windows else 両方で定義して、無条件呼び出しでいいかもしれない
非 Windows はそのまま返す関数で -
内部関数
s:hoge と関数を定義すると、モジュールの公開IFになるので、 s:_hoge として陰蔽したほうがいいのでは
(変数は平気)
モジュールの依存は
参考
vital.vim/autoload/vital/__vital__/System/Process.vim
Lines 7 to 22 in a93f5d3
There was a problem hiding this comment.
👍 > review comments
(とくに3、お願いします!)
| if !s:is_nvim | ||
| " inner callbacks for Vim | ||
| function! s:inner_out_cb(user_out_cb, ch, msg) abort | ||
| let result = a:msg | ||
| if s:is_windows | ||
| let result = s:iconv(a:msg, 'char', &encoding) | ||
| endif | ||
|
|
||
| call a:user_out_cb(result) | ||
| endfunction | ||
|
|
||
| function! s:inner_exit_cb(user_exit_cb, job, exit_code) abort | ||
| call a:user_exit_cb(a:exit_code) | ||
| endfunction | ||
|
|
||
| function! s:inner_err_cb(user_err_cb, ch, msg) abort | ||
| let result = a:msg | ||
| if s:is_windows | ||
| let result = s:iconv(a:msg, 'char', &encoding) | ||
| endif | ||
|
|
||
| call a:user_err_cb(result) | ||
| endfunction | ||
| else | ||
| " inner callbacks for Neovim | ||
| function! s:inner_out_cb(user_out_cb, job_id, data, event) abort | ||
| for line in a:data | ||
| if line !=# '' | ||
| call a:user_out_cb(line) | ||
| endif | ||
| endfor | ||
| endfunction | ||
|
|
||
| function! s:inner_exit_cb(user_exit_cb, job_id, exit_code, event) abort | ||
| call a:user_exit_cb(a:exit_code) | ||
| endfunction | ||
|
|
||
| function! s:inner_err_cb(user_err_cb, job_id, data, event) abort | ||
| for line in a:data | ||
| if line !=# '' | ||
| call a:user_err_cb(line) | ||
| endif | ||
| endfor | ||
| endfunction | ||
| endif |
There was a problem hiding this comment.
ここらへん、見える必要ないはずなので、これも陰蔽で
| if s:is_windows | ||
| let args = args + ['/c'] | ||
| else | ||
| let args = args + ['-c'] | ||
| endif |
There was a problem hiding this comment.
どちらかというと、シェル、そしてさらにOSとの組み合せで決りそうですが、大丈夫そう?
Linuxで pwsh とか
There was a problem hiding this comment.
ただ無理はしなくていいので、とりあえずいい感じの初期値でいいかとは思います。
There was a problem hiding this comment.
指摘の通りです。
cmd.exe が /c で、それ以外が -c となるように修正しました。
Commit: 7a6ce6d
| let args = args + [command] | ||
|
|
||
| let job_id = -1 | ||
| if s:is_nvim |
There was a problem hiding this comment.
いまはしなくていいですが、vim/nvimでサブモジュール相当(内部クラス分けでもいいかもですが)
していいくらいに、記述が共通じゃなく、分けて書いたほうが管理がよいかも
| let options = {} | ||
| let options['out_cb'] = function('s:inner_out_cb', [a:options.out_cb]) | ||
| let options['err_cb'] = function('s:inner_err_cb', [a:options.err_cb]) | ||
| let options['exit_cb'] = function('s:inner_exit_cb', [a:options.exit_cb]) |
There was a problem hiding this comment.
他もですが、まとめて辞書定義してもいいのではないかと
|
偉業 |
|
レビュー、コメント、ありがとうございます。 |
|
コールバックをすべて設定しないと |
mikoto2000
force-pushed
the
add-async-process
branch
from
d3fc57f to
7af35bb
Compare
問題を修正しました。 |
|
@mikoto2000 さらっと AsyncHTTP増えてますが、これの扱いが知りたいです ちゃんとするならドキュメントやテスト(このPRでやるかは別として) あると便利そうですが... |
|
すみません、うっかりまぜてしまいました。削除しました。 |
mikoto2000
force-pushed
the
add-async-process
branch
from
7af35bb to
4937a83
Compare
…allback was not set.
|
了解です。 |
3 participants
#836 の実装を行っています。
現時点でツッコミどころなど、指摘いただけると幸いです。
TODO:
-cor pwsh or powershell :-Cor cmd.exe :/c)