Skip to content

Conversation

@mikoto2000
Copy link
Contributor

close #842

@ujihisa ujihisa requested review from mattn and thinca December 21, 2025 00:20
@mikoto2000 mikoto2000 marked this pull request as draft December 23, 2025 14:02
@mikoto2000
Copy link
Contributor Author

Web.HTTP, Web.AsyncHTTP, Web.HTTP.Core に分割していこう、 Vitalize に失敗するようになっていたのでドラフトに戻しました。

@mikoto2000 mikoto2000 marked this pull request as ready for review December 23, 2025 14:46
@mikoto2000
Copy link
Contributor Author

Vitalize に失敗する問題を修正したのでドラフトを解除しました。

Copy link
Contributor

@tsuyoshicho tsuyoshicho left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mikoto2000
確認できた改善点をコメントしました。
一部は修正必須と思うので(といってもtypo) Request changes にしました。

This function requires one of the clients, "curl" or "wget".

Example: >
let s:AsyncHTP = vital#{plugin-name}#new().import('Web.AsyncHTTP')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
let s:AsyncHTP = vital#{plugin-name}#new().import('Web.AsyncHTTP')
let s:AsyncHTTP = vital#{plugin-name}#new().import('Web.AsyncHTTP')

typo fix

Comment on lines 194 to 196
python *Vital.Web.AsyncHTTP-client-python*
"python" as "python3" or "python2" auto select.
(High priority for "python3".)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Core で Python のファイル依存をはずしたりしてますが、こちらは利用できますか?

(CoreとPython利用の組み合せがどうなっているかを確認したい感じです)

もし、非同期との関係を考えて、サポートしないほうがいいのであれば、clientから外すのはアリと思いますので、記述を更新してもらえれば

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

client の実体は、 HTTP, AsyncHTTP それぞれに存在しています。 (同期・非同期で実装が変わるため、それぞれで定義)
そのため、 python への依存は HTTP, AsyncHTTP それぞれから行っています。
ですので、 Core から python への依存は無くても大丈夫です。

Linux でですが、以下コードもエラーなく動きます。

let s:AsyncHTTP = vital#vital#import('Web.AsyncHTTP')

function! s:user_cb(response) abort
  echomsg a:response
  " => Web.HTTP と同じ形式の辞書
endfunction

call s:AsyncHTTP.request({
      \ 'url': 'https://example.com',
      \ 'user_cb': function('s:user_cb'),
      \ 'clients': ['python2'],
      \ })

call s:AsyncHTTP.request({
      \ 'url': 'https://example.com',
      \ 'user_cb': function('s:user_cb'),
      \ 'clients': ['python3'],
      \ })

function! s:_vital_loaded(V) abort
let s:V = a:V
let s:Prelude = s:V.import('Prelude')
let s:HTTP = s:V.import('Web.HTTP')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

この依存関係は削除して、利用している機能は Core に置き、それを HTTP/AsyncHTTP が利用するように改善したほうがいいかと

Comment on lines 257 to 258
if has_key(s:clients.curl.errcode(), retcode)
throw 'vital: Web.HTTP: ' . s:clients.curl.errcode()[retcode]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

せっかくのリファクタリングでもあるので

if s:clients.curl.errcode_has_key(retcode)
      throw 'vital: Web.HTTP: ' . s:clients.curl.errcode(retcode)

みたいなインタフェースにしてもいいかもしれませんね

errcode()が配列返すのがマストではないし、複数回利用するようなら、配列を初期化時にコピー相当でもいいので、

  • コピーする
  • 関数IFにする

のどちらかに寄せたほうがいいかなと

@mikoto2000
Copy link
Contributor Author

クライアントの指定方法を感知餓死していたため、 curl 以外の動作確認ができていない状態でした。
今正しい方法で curl 以外の動作確認をしたらエラーになる状態でしたので、またドラフトに戻します...

@mikoto2000 mikoto2000 marked this pull request as draft December 25, 2025 12:59
Comment on lines 137 to 148
parseHeader({headers}) *Vital.Web.HTTP.parseHeader()*
Parse {headers} list to a dictionary.
Duplicated fields are overwritten.

encodeURI({param}) *Vital.Web.HTTP.encodeURI()*
Encode params as URI query.

decodeURI({str}) *Vital.Web.HTTP.decodeURI()*
Decode string as URI params.

encodeURIComponent({str}) *Vital.Web.HTTP.encodeURIComponent()*
Encode param as URI components.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ここらへん AsyncHTTP になってないため、タグ重複エラーしてるみたいです。

正式で出すにあたって、修正しておいてください。

@mikoto2000
Copy link
Contributor Author

リカバリ力が足りず、一から細かく刻みつつ再実装してしまいました。
せっかくレビューいただいたのに申し訳ありません...。
指摘は反映させたつもりです。

@mikoto2000 mikoto2000 marked this pull request as ready for review December 26, 2025 23:41
Copy link
Contributor

@tsuyoshicho tsuyoshicho left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

見てみました

CLIENT *Vital.Web.AsyncHTTP-client*

The following can be used.
(TODO: More document. Especially about limitation.)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

これは削除していいと思います。

また、Python系について、Pythonコマンド+スクリプトファイル+引数を AsyncProcess するという方法はなくはないので、一応TODOとしてPython3対応は将来予定として記載しておくのはどうでしょう?

(というか、そろそろ Python2 はもういい気がするな...w)

"unixSocket" Default: (None)
Use --unix-sokect (only curl >= 7.40.0)

"user_cb" Default: (None)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ここだけスネークケースなのは微妙なので

Suggested change
"user_cb" Default: (None)
"userCallback" Default: (None)

という引数にするのはどうでしょうか?

(そもそも Vim の記法としてどうかはあるけど)

return result
endfunction

" public interface implements
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

うえの build 系なども利用はあるので、そちらも public な気がします。

逆に Core 内に閉じる関数もなくない気がしますが... (あったら、やるなら s:_hoge 化?)

なお関数の記法がいろいろなのは、内部処理だったからでしょうね、そこはとりあえずそのままでもいいと思います。

Copy link
Contributor

@tsuyoshicho tsuyoshicho left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

おつかれさまです、安定したと思うので、詳細に見てみました。

ちょっと迷走しましたが、settings の client と、urlencode_char 、 依存関係のところは対処が必要と思います。

let settings = {
\ 'method': 'GET',
\ 'headers': {},
\ 'client': ['python', 'curl', 'wget', 'python3', 'python2'],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

クライアントは各実装依存となったので、こちらで固定で設定するのではなく

  • request で追加設定する
  • 引数を追加して、引数から client 設定する

などのAPI設計にしたようが良いと思われます。

Copy link
Contributor

@tsuyoshicho tsuyoshicho left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

追加指摘です。

一応helpは
https://github.com/vim-jp/vimdoc-ja-working/wiki/Guide
のフォーマッタ設定を使うといいのですが、現状ちゃんと書いてあるから問題はないといえばないです

やたら差分でますが、調整の範囲ではあるという...

Comment on lines 3 to 4
Maintainer: mattn <[email protected]>
thinca <[email protected]>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ここは自分にしてください
(元にしたのが Web.HTTP と書いてもいいかもです

@mikoto2000
Copy link
Contributor Author

一応helpは
https://github.com/vim-jp/vimdoc-ja-working/wiki/Guide
のフォーマッタ設定を使うといいのですが、現状ちゃんと書いてあるから問題はないといえばないです

やたら差分でますが、調整の範囲ではあるという...

82ad926 で対応いたしました。
Web.HTTP をベースに書いたのですが、あえて改行を入れている箇所以外はフォーマッタと同じ結果になるようでした。
AsyncHTTP としたせいで行幅を越していた 1 行を修正しています。

Copy link
Contributor

@tsuyoshicho tsuyoshicho left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

たぶんいいとは思います

(Coreのテストは悩むところ)

to: @thinca @mattn
レビューおねがいします。
あるいは他にレビュー向きな人がいれば、そちらへ ping していただければ

(非同期だと lambdalisue さんとかだろうか...)

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Web.HTTP の非同期実行を行いたい

2 participants