Backport System.Job and System.Process.Job #513

lambdalisue · 2017-05-20T09:07:47Z

from https://github.com/lambdalisue/vital-System-Job

ついでに Travis で Neovim のテストも走らせています。
現状は System.Job だけです。

tyru · 2017-05-20T09:09:54Z

https://github.com/lambdalisue/vital-System-Job は MIT ライセンスみたいですけど vital に入れるなら NYSL になると思いますが、大丈夫ですかね？

lambdalisue · 2017-05-20T09:10:35Z

はい。そのつもりでライセンス部分消しました。

vim-jp-bot · 2017-05-20T09:12:15Z

autoload/vital/__vital__/System/Process/Job.vim

+  call extend(self._content, [leading . get(a:msg, 0, '')] + a:msg[1:])
+endfunction
+
+function! s:stream.on_stderr(job, msg, event) abort


[vimlint] _{reported by reviewdog 🐶}
EVL103: unused argument a:job

vim-jp-bot · 2017-05-20T09:12:16Z

autoload/vital/__vital__/System/Process/Job.vim

+" Stream ---------------------------------------------------------------------
+let s:stream = {}
+
+function! s:stream.on_stdout(job, msg, event) abort


[vimlint] _{reported by reviewdog 🐶}
EVL103: unused argument a:event

vim-jp-bot · 2017-05-20T09:12:16Z

autoload/vital/__vital__/System/Process/Job.vim

+" Stream ---------------------------------------------------------------------
+let s:stream = {}
+
+function! s:stream.on_stdout(job, msg, event) abort


[vimlint] _{reported by reviewdog 🐶}
EVL103: unused argument a:job

vim-jp-bot · 2017-05-20T09:12:16Z

autoload/vital/__vital__/System/Job.vim

+        call s:_on_msg_cb(a:name, a:job, a:job.__channel, msg)
+      endif
+      sleep 1m
+      let status = ch_status(a:job.__channel, a:options)


[vimlint] _{reported by reviewdog 🐶}
E118: Too many arguments for function: ch_status

vim-jp-bot · 2017-05-20T09:12:16Z

autoload/vital/__vital__/System/Job.vim

+          \ 'timeout': 10000,
+          \ 'out_cb': function('s:_on_msg_cb', ['stdout', job]),
+          \ 'err_cb': function('s:_on_msg_cb', ['stderr', job]),
+          \ 'exit_cb': function('s:_on_exit_cb', [job]),


[vimlint] _{reported by reviewdog 🐶}
E118: Too many arguments for function: function

vim-jp-bot · 2017-05-20T09:12:16Z

autoload/vital/__vital__/System/Process/Job.vim

+  call extend(self._content, [leading . get(a:msg, 0, '')] + a:msg[1:])
+endfunction
+
+function! s:stream.on_stderr(job, msg, event) abort


[vimlint] _{reported by reviewdog 🐶}
EVL103: unused argument a:event

vim-jp-bot · 2017-05-20T09:12:16Z

autoload/vital/__vital__/System/Job.vim

+          \ 'mode': 'raw',
+          \ 'timeout': 10000,
+          \ 'out_cb': function('s:_on_msg_cb', ['stdout', job]),
+          \ 'err_cb': function('s:_on_msg_cb', ['stderr', job]),


[vimlint] _{reported by reviewdog 🐶}
E118: Too many arguments for function: function

vim-jp-bot · 2017-05-20T09:12:16Z

autoload/vital/__vital__/System/Job.vim

+    let job_options = {
+          \ 'mode': 'raw',
+          \ 'timeout': 10000,
+          \ 'out_cb': function('s:_on_msg_cb', ['stdout', job]),


[vimlint] _{reported by reviewdog 🐶}
E118: Too many arguments for function: function

vim-jp-bot · 2017-05-20T09:12:16Z

autoload/vital/__vital__/System/Job.vim

+  endfunction
+
+  function! s:_read_channel_and_call_callback(job, name, options) abort
+    let status = ch_status(a:job.__channel, a:options)


[vimlint] _{reported by reviewdog 🐶}
E118: Too many arguments for function: ch_status

ujihisa · 2017-05-20T09:18:15Z

doc/vital/System/Job.txt

+INTRODUCTION					*Vital.System.Job-introduction*
+
+*Vital.System.Job* is a wrapper module of job feature which has introduced
+from Vim 8 or Neovim.


to provide an abstract layer that works both for Vim 8 and Neovim at the same time とかみたいに両方イけることを強調するとなお良さそう

ujihisa · 2017-05-20T09:18:59Z

doc/vital/System/Job.txt

+=============================================================================
+USAGE						*Vital.System.Job-usage*
+
+The following code start "git status" and store its' stdout/stderr output in


s/'//
s/start/starts/
s/store/stores/

ujihisa · 2017-05-20T09:19:37Z

doc/vital/System/Job.txt

+	"on_exit"	A callback called when the process terminated.
+			The {msg} is an exit status.
+
+	Note that all attributes in the {options} is exposed to the job


s/is/are/

lambdalisue · 2017-05-20T09:44:19Z

だいたいレビュー揃ってから修正開始します。
一応これ以後は最後の最後以外は --force しないのでクローンOKです

ujihisa · 2017-05-20T12:15:32Z

方針は超LGTM
明日手元で実際に使ってあれこれしてみよう

tyru · 2017-05-21T09:27:07Z

@haya14busa @thinca このモジュールって Vim 8 か NeoVim でしか動かないと思うのですが、そういうモジュールの場合、import() 時にエラーになる仕組みってないんでしたっけ？
例えばユーザの Vim が 7.x で import() した時に
Vim 8 or NeoVim is required みたいな例外を投げたい場合、s:_vital_created() で throw すればそのまま import() 時に上位に伝わったりしますかね？
あとそれが投げたい場合の正式な手順としてサポートすべきかという話も含めて相談したいです。

tyru · 2017-05-21T09:27:56Z

オフトピな話題ですみません。
必要ありそうなら別 issue 立ててそこで議論したい…

ujihisa · 2017-05-21T10:14:52Z

@haya14busa @thinca このモジュールって Vim 8 か NeoVim でしか動かないと思うのですが、そういうモジュールの場合、import() 時にエラーになる仕組みってないんでしたっけ？

現状import()時ではなく、is_available()などを明示的に呼び出して利用可能か確認するモジュールならある、という感じです (vimprocに依存してるConcurrentProcessなど)。あちらであの仕様にしたのは、「importはトップレベルでとにかく無条件に行って、プラギンの各関数でis_availableして、プラギンごとによしなに対処する」みたいなつくりが実用上便利だったからです。

今後この方針を変えるかどうか一度ちゃんと議論するのはよさそう。もちもん、それはこのpullreqとは別の独立したissueでだけど

tyru · 2017-05-21T15:53:40Z

@ujihisa なるほど…ありがとうございます。issue 作っちゃいました。 #515

rhysd · 2017-06-06T13:48:55Z

.travis.yml

  - ruby scripts/check-changelog.rb
+  - bash scripts/script.sh


script.sh はファイル名が何をするスクリプトか分かるように改名したほうが良いかもです．

rhysd · 2017-06-06T13:52:52Z

autoload/vital/__vital__/System/Job.vim

+
+  function! s:job.wait(...) abort
+    let timeout = get(a:000, 0, v:null)
+    if timeout is# v:null


nit: ここは v:null を持ち出さなくても

if a:0 > 0 return jobwait([self.__job], a:1)[0] else return jobwait([self.__job])[0] endif

で良いかもです

その場合 s:job.wait(v:null) と明示的に指定できなくなるので少し表現力が下がってしまいます。
例えば s:job.wait(...) を内部的に呼び出す別の関数が会った場合 v:null を渡してもOKという状況のほうが使い勝手が高いと思います。

rhysd · 2017-06-06T13:56:28Z

autoload/vital/__vital__/System/Job.vim

+        let msg = ch_read(a:job.__channel, a:options)
+        call s:_on_msg_cb(a:name, a:job, a:job.__channel, msg)
+      endif
+      sleep 1m


なぜ 1m 待っているのかコメントがあると嬉しいかもです

rhysd · 2017-06-06T14:02:25Z

autoload/vital/__vital__/System/Job.vim

+    " Start job and return a job instance
+    let job.__job = jobstart(a:args, job)
+    let job.__status = job.__job > 0 ? 'run' : 'dead'
+    let job.args = a:args


この args プロパティは後で使っていない気がします

前口頭で説明しましたが、この args は後で参照できるように残してあります

rhysd · 2017-06-06T14:10:52Z

autoload/vital/__vital__/System/Process/Job.vim

+
+function! s:is_supported(options) abort
+  if get(a:options, 'background') && (
+        \   s:Prelude.is_string(get(a:options, 'input')) ||


Prelude は一応廃止じゃなかったでしたっけ？（CC: @ujihisa）もしそうなら上記の s:t_list みたいに自前でチェックしたほうが良いかもしれないです．

Type もあるし、とりあえずこれはこのままで行きます〜

rhysd · 2017-06-06T14:12:08Z

autoload/vital/__vital__/System/Process/Job.vim

+      echomsg printf(
+            \ 'vital: System.Process.Job: %s',
+            \ cmdline
+            \)


nit: ここは . で文字列結合で良い気も．

echomsg 'vital: System.Process.Job: ' . cmdline

lambdalisue · 2017-07-01T09:55:32Z

やっと Travis 氏倒した……

再レビューお願い致します

vim-jp-bot · 2017-07-01T10:00:34Z

autoload/vital/__vital__/System/Job.vim

+      endif
+      " Without sleep, Vim would hung
+      sleep 1m
+      let status = ch_status(a:job.__channel, a:options)


[vimlint] _{reported by reviewdog 🐶}
E118: Too many arguments for function: ch_status

lambdalisue · 2017-07-01T12:23:21Z

倒せてなかったorz

Backport System.Job and System.Process.Job from https://github.com/lambdalisue/vital-System-Job To fix travis for Neovim, the following are also performed - Use 'trusty' instead to upgrade cmake newer - Add requirements for build Neovim - Fix test scripts - Remove unnecessary flags to reduce dependencies - --enable-perlinterp - --enable-rubyinterp

lambdalisue · 2017-07-01T14:30:44Z

drone で client9/misspell のビルドにコケてるけど beego/wetalk#32 っぽい？

[info] Pulling image plugins/drone-git:latest
Drone Git Plugin built from 43dcd64
$ git init
Initialized empty Git repository in /drone/src/github.com/vim-jp/vital.vim/.git/
$ git remote add origin https://github.com/vim-jp/vital.vim.git
$ git fetch --no-tags origin +refs/pull/513/merge:
From https://github.com/vim-jp/vital.vim
 * branch            refs/pull/513/merge -> FETCH_HEAD
$ git checkout -qf FETCH_HEAD
$ go get -d -v ./scripts
github.com/haya14busa/go-vimlparser (download)
$ go get -u github.com/client9/misspell/cmd/misspell
go build github.com/client9/misspell: /usr/lib/go/pkg/tool/linux_amd64/compile: signal: killed
[info] build failed (exit code 1)

lambdalisue · 2017-07-01T14:41:55Z

@haya14busa ちょっと僕だと drone コケてるの直せないので確認していただけませんか？

haya14busa · 2017-07-02T11:11:38Z

job restartしたら green になりました．が，これは根本的ソリューションではない...

スペック低すぎてリソース足りなくて misspell ビルドできないってケースなのかもしれないですね... っらぃ...

rhysd · 2017-07-02T22:04:39Z

@haya14busa ビルドするのやめてバイナリ置いとくだけにします？ misspell は割りと実装がゴリ押しなのでそのせいでビルドにメモリ食うのかも…

lambdalisue · 2017-07-02T23:06:50Z

ビルドで落ちるの健康に悪い（ビルドで落ちたら嫌だからコミットしたくないとか）のでバイナリ置けるのであればそうして頂けると助かりますー

k-takata · 2017-07-02T23:18:49Z

公式のnightly buildはどうでしょうか。
https://github.com/neovim/bot-ci#setting-up-integration-builds

lambdalisue · 2017-07-02T23:24:04Z

お、misspell とは違いますが、確かに neovim のビルドこれ使った方が良いかもですね。週末詳しくみてみます

Building misspell might use lots of resource and be killed. #513 (comment) ref: https://github.com/client9/misspell#install

While non of code use rubyinterp. I simply removed it.

lambdalisue · 2017-07-23T04:45:01Z

クソ面倒クセェ！！！

Wait a bit before job.status() is called

lambdalisue · 2017-11-04T13:36:51Z

ちょっと思う所あるから一旦おろします。

vim-jp-bot reviewed May 20, 2017

View reviewed changes

lambdalisue force-pushed the system-job branch from 632d60e to 24c10e5 Compare May 20, 2017 09:15

ujihisa reviewed May 20, 2017

View reviewed changes

lambdalisue force-pushed the system-job branch from 24c10e5 to ab5dc45 Compare May 20, 2017 09:32

lambdalisue changed the title ~~WIP: Backport System.Job and System.Process.Job~~ Backport System.Job and System.Process.Job May 20, 2017

lambdalisue requested review from ujihisa, thinca and rhysd May 20, 2017 09:43

tyru mentioned this pull request May 21, 2017

module import() behavior where the module only supports specific environments #515

Open

rhysd reviewed Jun 6, 2017

View reviewed changes

rhysd suggested changes Jun 6, 2017

View reviewed changes

rhysd reviewed Jun 6, 2017

View reviewed changes

lambdalisue force-pushed the system-job branch from f17deb9 to a3d0b72 Compare July 1, 2017 09:52

vim-jp-bot reviewed Jul 1, 2017

View reviewed changes

lambdalisue force-pushed the system-job branch from 1a422b3 to 0eaaca1 Compare July 1, 2017 14:17

lambdalisue force-pushed the system-job branch from 0eaaca1 to 2ece69e Compare July 1, 2017 14:18

haya14busa added a commit that referenced this pull request Jul 5, 2017

CI: Use misspell binary

97bfae0

Building misspell might use lots of resource and be killed. #513 (comment) ref: https://github.com/client9/misspell#install

haya14busa mentioned this pull request Jul 5, 2017

CI: Use misspell binary #529

Merged

haya14busa added a commit that referenced this pull request Jul 5, 2017

CI: Use misspell binary

4ee12f4

Building misspell might use lots of resource and be killed. #513 (comment) ref: https://github.com/client9/misspell#install

lambdalisue added 2 commits July 18, 2017 22:24

Merge branch 'master' into system-job

6b265fd

Add ruby-dev

18a98eb

lambdalisue mentioned this pull request Jul 22, 2017

Process: system() should use jobstart() on neovim #533

Open

lambdalisue added 2 commits July 22, 2017 20:54

Use libruby instead

bf27691

Remove ruby

055aac8

While non of code use rubyinterp. I simply removed it.

lambdalisue added 2 commits July 23, 2017 13:46

Add ruby on Mac

4991059

System.Job: Fix test

255cb5a

Wait a bit before job.status() is called

lambdalisue mentioned this pull request Jul 23, 2017

Remove --rubyinterp　とか #534

Closed

lambdalisue mentioned this pull request Aug 2, 2017

WIP: Refactoring & Fix issue on nvim tyru/open-browser.vim#101

Open

lambdalisue closed this Nov 4, 2017

thinca deleted the system-job branch December 17, 2017 13:52

Backport System.Job and System.Process.Job #513

Backport System.Job and System.Process.Job #513

Conversation

lambdalisue commented May 20, 2017 • edited Loading

tyru commented May 20, 2017

lambdalisue commented May 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ujihisa May 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lambdalisue commented May 20, 2017

ujihisa commented May 20, 2017

tyru commented May 21, 2017

tyru commented May 21, 2017

ujihisa commented May 21, 2017

tyru commented May 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lambdalisue commented Jul 1, 2017

Choose a reason for hiding this comment

lambdalisue commented Jul 1, 2017

lambdalisue commented Jul 1, 2017 • edited Loading

lambdalisue commented Jul 1, 2017

haya14busa commented Jul 2, 2017

rhysd commented Jul 2, 2017

lambdalisue commented Jul 2, 2017

k-takata commented Jul 2, 2017

lambdalisue commented Jul 2, 2017

lambdalisue commented Jul 23, 2017

lambdalisue commented Nov 4, 2017

lambdalisue commented May 20, 2017 •

edited

Loading

ujihisa May 20, 2017 •

edited

Loading

lambdalisue commented Jul 1, 2017 •

edited

Loading