Change/surf to reqwest #788
Conversation
crates/test_util/build.rs
Outdated
| async fn main() -> anyhow::Result<()> { | ||
| let mut dist = PathBuf::from(env::var_os("CARGO_MANIFEST_DIR").unwrap()); | ||
| dist.push("data"); | ||
|
|
||
| let dic_dir = dist.join(DIC_DIR_NAME); | ||
|
|
There was a problem hiding this comment.
ここと50行目の空行は削除できないでしょうか (44行目のは消してもそのままでも)。
Squash and mergeの方針の場合1PR=1コミットになるので、PRの主旨から外れる変更はできるだけ避けた方がよいというのが一般的な慣習かなと思っています。
There was a problem hiding this comment.
了解しました 空行は削除可能です
PRの趣旨から外れる、というのは全体の依存関係からsurf等を削除したことについてで合っているでしょうか？
There was a problem hiding this comment.
そうですね。ここの空行の追加はsurf→reqwestの置き換えに関連しないと思います。
まあ避けた方がよいというのも程度と状況によりますし、diffを気にするあまりコードの状態が悪くなるのは本末転倒なのですが、このPRのような変更ならばコミットログを綺麗にする方に倒した方がよいかなと思った次第です。
There was a problem hiding this comment.
- 不必要に追加された空行の削除
- 依存関係の削除を，全体ではなくtest_utilクレート内のみに変更
を行いました
依存関係の変更はあくまでtest_util内のみにとどめた方がよいかと思いましたが，issueがtokioランタイムに統一という趣旨なのでワークスペースの依存関係からsurfとasync-stdを削除するコミットを残したほうがよかったでしょうか...
There was a problem hiding this comment.
すみません誤読していました 🙇
趣旨から外れるのは空行のところで、surfの排除自体は #777 に沿うと思います。git revert HEAD^^..していただければ！
There was a problem hiding this comment.
了解しました、ありがとうございます！
There was a problem hiding this comment.
- ワークスペースの依存関係から
surfとasync_stdを削除
を行いました
There was a problem hiding this comment.
お手数をおかけしましたが、LGTM! Cargo.lockが綺麗に900行近く小さくなりましたね。
approval×1でマージします。
CC: @Hiroshiba
内容
crates/test_util/build.rs内で使用されているsurfをreqwestに置き換えasync_std::mainをtokio::mainに変更test_util/Cargo.tomlにtokioとreqwestのビルド時依存関係，必要なfeaturesを追加async-stdとsurfを全体の依存関係から削除reqwestクレートはデフォルトでリダイレクトを10件まで追跡するらしい．surf使用時にあったリダイレクトミドルウェアをクライアントに付加するコードに相当する部分はreqwestでは不要と判断．
関連 Issue
その他