vital.vim icon indicating copy to clipboard operation
vital.vim copied to clipboard

Published 20 hours ago • verified publisher icon vim-jp

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

Open tyru opened this issue 7 years ago • 12 comments

https://github.com/vim-jp/vital.vim/pull/513#issuecomment-302925402 から派生。

要約

特定環境でのみ動くモジュールを import() した時、vital のモジュール全般においてどのような挙動にするべきかを話し合いたいです。 今出ている案は以下の通りです。

  1. import() 時に例外を投げる
  2. import() 時には例外を投げず、ユーザにチェック用メソッドを呼んでもらってチェックしてもらう

本文

#513 のモジュールの様に Vim 8 か NeoVim でしか動かないモジュールの場合、 サポートされない環境で import() した時に Vim 8 or NeoVim is required みたいな例外を投げたい場合もあるかと思いました。 しかし ujihisa さん曰く、ConcurrentProcess ではそのようになっていません（ユーザに is_available() でチェックしてもらう API になっている）。 インターフェースが統一されてるといいかと思ったので、vital ではどのようにすべきかを相談したいです。

  1. import() 時に例外を投げる
    • メリット：チェック用メソッドを呼び忘れることがない
    • デメリット：（2 の案のメリットの反対）import() をトップレベルで無条件に行うことができない
  2. import() 時には例外を投げず、ユーザにチェック用メソッドを呼んでもらってチェックしてもらう
    • メリット：import() をトップレベルで無条件に行うことができる（catch する必要がない）
    • デメリット：（1 の案のメリットの反対）チェック用メソッドを呼び忘れてしまう可能性がある

tyru avatar May 21 '17 15:05 tyru

この問題に関しては二種類あると思います。

  1. バージョンの違いによりモジュール全体が使えなくなる場合
  2. バージョンの違いによりモジュールの一部の機能が使えなくなる場合

今回は話をシンプルにするために 1. だけ考えます（ 2. に関しても考えたほうが良さそうですが、たぶん別 issue ）

全体的に使えなくなる場合、インポートの意味すら無いため import() 時に例外を投げてくれたほうが使いやすいです。 ただ、後方互換にならないため

let s:Job = vital#vital#import("System.Job")  " 例外を投げる
let s:Job = vital#vital#import("System.Job", {'fail_silently': 1})  " 例外を投げない

な使い方が出来ると良いと思いました。なお例外を投げる方をデフォルトにしているのは、例外をデフォルトにしても壊れるプラグインが少ないかな？という個人的な感想によるものです。

lambdalisue avatar May 22 '17 00:05 lambdalisue

1. import() 時に例外を投げる かつlambdalisueさんみたいに指定できる方針に一票。具体的にはそもそも関数名も変えるのがよさそう。後方互換性は壊して、かわりに、既存のコードは以下のような機械的な変換をすればすれば動くよ、という感じで。

let s:Job = vital#vital#import("System.Job")  " 例外を投げる
let s:Job = vital#vital#import_with_no_availability_check("System.Job")  " 例外を投げない

(後者の関数名が長く冗長なのは意図的です)

現状、ConcurrentProcessのようにis_available()があるモジュールは以下です。

  • Database/SQLite.vim
  • ConcurrentProcess.vim
  • System/Process/System.vim
  • System/Process/Vimproc.vim
  • System/Process/Mock.vim " つねに1
  • Deprecated/ProcessManager.vim " 無視してok

ujihisa avatar May 22 '17 02:05 ujihisa

モジュールに.is_available()があれば自動的にimport()でそれ呼んで、falseなら例外投げるの良さそう

aiya000 avatar May 22 '17 14:05 aiya000

長く冗長な関数名に一票！

lambdalisue avatar May 24 '17 03:05 lambdalisue

自分も import() 時の挙動としては @lambdalisue さんと @ujihisa さんの挙げた import() 時に例外を投げる 案に同意です。 長い関数名については…このくらいの方が import() が並んでても見分けやすいのかも？（ちょっと長過ぎる気もしますが） 関数名はともかく、自分も別の名前の関数を提供する、に一票です。

tyru avatar May 24 '17 10:05 tyru

うーん.... これは好みかもしれないですが、import 時に例外を投げるのはともかく、引数増やしてコントロールしたり別名つくってまでやるのは好きじゃないですね... それやるなら try catch でいいし....

個人的には is_available とかのほうがまだよい。全体のモジュールが使えないのか、特定のメソッドだけ使えないのかもコントロールできるし。(ここでは全体がダメな時と一部がダメなときとの一貫性も考えるという意)

という意味で例外投げるのも厳しい気持ち。たとえば今後job関連の機能が追加されたり挙動が変更されるとしてメソッドが増えたり一部のメソッドが変わったりする場合、(neovim or vim8) & has_patch(xxx) となり、それで全体のimport自体を例外投げるようにするか?と言われたら絶対Noだと思う。

あと、

チェック用メソッドを呼び忘れることがない

というのはまぁ確かにメリットではあるとは思うんですが、チェックしたところで結局動かないのは変わらないので、例外メッセージをわかりやすくする程度のメリットしかないんですよね。 それが大事ではあるとは思うんですが僕としてはそこまでしていれたい機能ではない.

vital 以外のビルトイン関数の仕様変更などは結局バージョンなどプラギン開発者がチェックしてるので、それと同じ感じで開発者がある程度やれるしコントローラブルだと嬉しい。

一番好みなのは理想案ではないですが、ドキュメントにしっかり条件をかいてあればそれが一番うれしい。モジュール全体としてのrequirementsはこうで、このメソッドはこういう条件があって...など. それプラスで条件が複雑だったりするときは is_available とかチェック系関数を用意してもらえば嬉しくて、import時に勝手に"いい感じ"のエラーメッセージをthrowされるのよりは断然すきです。

haya14busa avatar Jun 02 '17 10:06 haya14busa

throwしない用の施工をするよりはtry-catchすればいい……というのはDRY原則に反するかなーと思います……:dog:

aiya000 avatar Jun 04 '17 07:06 aiya000

想定ケースの違いですかね。僕は

  1. インポートする意味がない場合
  2. 一部の機能が使えない場合

の二種があると思って居ます。はやぶささんが仰る通り、例外に関しては 1. の対応しか出来ない上に 2. に対して別解を用意する必要があり一貫性がありません。

ただ、僕が想定しているケースとして「とりあえずそれっぽい名前があるからインポートして使っている」程度の認識を想定して居ます。この場合、インポート時に例外が発生すれば比較的早い段階で使えないことが分かるので、なんだかわからないけどエラーが出てプラグインが動かない」って状況を避けれるかなと思っています。

あと try-catch に関しては懐疑的です。引数や関数名であれば機械的に変換出来ますが try-catch だと厳しいです（そもそも後方互換はあくまでも利便性の為であり、推奨はエラーを出す方なので）

lambdalisue avatar Jun 04 '17 09:06 lambdalisue

コードをベースに話をしたほうが良いかなと思います．

1. import が例外を投げる時

try
  let s:Job = vital#some_plugin#import('System.Job')
catch
endtry

" 処理を分けるところ
if exists('s:Job')
  " job があるとき〜
else
  " ないとき…
endif

2. import が例外を投げない時

let s:Job = vital#some_plugin#import('System.Job')

if s:Job.is_available()
  " job があるとき〜
else
  " ないとき…
endif

この2つを比較してみると，確かに try ... catch を忘れていた場合，例えば Vim7 で 1. のパターンを実行するとロード時にエラーになってくれるのでうれしいというのは分かります．しかし，「例外を投げたこと」を覚えておかないといけなくて，結局 exists('s:Job') みたいに別のチェックを挟む必要があり，ここを忘れると結局同じ問題に落ちる気がします．コード的にチェック箇所が減るわけでないのに try ... catch（なり別の import 関数呼ぶなり）でコードも冗長になりますし…

let s:Job = vital#vital#import("System.Job") " 例外を投げる let s:Job = vital#vital#import_with_no_availability_check("System.Job") " 例外を投げない

僕も try ... catch で明示的に例外をハンドルするのが良いと思います．

モジュールに.is_available()があれば自動的にimport()でそれ呼んで、falseなら例外投げるの良さそう

こういう暗黙の挙動はデメリットしか感じないんですが，どう良いんでしょう？

rhysd avatar Jun 06 '17 13:06 rhysd 

try
  let s:Job = vital#some_plugin#import('System.Job')
  function! foo(...)
    " あるとき〜
  endfunction
catch
  function! foo(...)
    " ないとき…
  endfunction
endtry

みたいに関数切り分けるとかはありかもですが，プラグインの処理によってはこういう実装は厳しそうな気も

rhysd avatar Jun 06 '17 13:06 rhysd

確かに投げる時と投げない時でエラーをハンドリングしなきゃいけないことに変わりはないのであまり変わらなそうですね。

関数名はともかく、自分も別の名前の関数を提供する、に一票です。

これは撤回で、vital#some_plugin#import() が例外を投げるようになるだけでいい、というのに賛成です。

tyru avatar Jun 06 '17 14:06 tyru

昨日の話し合いとして、頑なに例外を推していたのは自分だけだったのですが

  1. 例外を採用すると必ず try が必要になってしまう
  2. 逆に例外ではなくチェックであれば、例外を投げる選択肢を自分の裁量で採用できる
  3. 例外もチェックもそれぞれ利点があって比較は難しい

の三点から余力を残すために例外を採用しないでチェックで対応するという形で同意しました。

また、ここで言われている is_availabe() による真偽のテストではなく vital#plugin_name#health_check('module name') にて最低でも以下の情報を含む辞書を返す形で同意が取れています（たぶん

  • status : boolean, 使用可能か否か。一部使用可能な場合に true を返すかはモジュール作者の思惑依存
  • reason : str, 使えない場合は理由。そのままエンドユーザーに表示する想定

上記に加え、一部利用できない関数などがあれば追加フィールドとしてモジュール作者に依存する

また vital#plugin_name#health_check() を実行した場合は上記辞書を子要素に持ち、（プラグインが依存する）各モジュール名をキーとする辞書を返すことで、プラグインの全依存関係が一度で取得できる。

各位上記に認識の相違があれば突っ込みお願いします

lambdalisue avatar Jun 09 '17 15:06 lambdalisue