fix: Docker関連ファイルの削除とTyposのスクリプトの追加 #2239
Conversation
There was a problem hiding this comment.
いくつか意見です。
1：
他に使いまわせるようにするかtypos専用にするか、が中途半端な気がします。
というのも：downloadTypos.jsの今のコードは、
- Windows用のバイナリは全て7zで処理できて、
- Mac/Linuxならtarで処理できる
という前提の処理で書かれていると思います。
これに汎用性を持たせるのはまぁしんどいと思うので、typos専用にした方が楽になるかも。
2：
ダウンロードURLをハードコードすれば、変にバージョンが上がったりしないので後々困らなくなりそう。（解決されたバージョンが固定できないなら動的なバージョン指定はやるべきではないと言う思想）
const urls = {
windows: {
x64: "https://...",
},
darwin: {
...
},
linux: {
...
}
}3：
Workflow（.github/workflows下）のtyposダウンロードは無くしていいかも。
package.json
Outdated
| @@ -128,4 +129,4 @@ | |||
| "vue-tsc": "2.0.24", | |||
| "yargs": "17.2.1" | |||
| } | |||
| } | |||
| } | |||
build/vendored/README.md
Outdated
| | ディレクトリ名 | 内容 | ダウンローダー | | ||
| | -------------- | ------------------------------------------ | ------------------------ | | ||
| | `7z` | [7-Zip](http://www.7-zip.org/) | `build/download7z.js` | | ||
| | `typos` | [typos](https://github.com/crate-ci/typos) | `build/downloadTypos.js` | |
There was a problem hiding this comment.
他に使いまわせるようにするかtypos専用にするか
難しいですが、条件に寄ってはどちらもありに感じています。
他に使い回せるようにするなら、実際に使いまわしたいですね！
たぶんtyposと同様にshellcheckのダウンロードもできた方が良いので、やるならこっちもかなと。
（そして試してみたら共通化がかなり難しいことに気づくと思います）
提案としては、こうとかどうでしょう？
- まずtypos専用にシンプルにdownloadTyposを書いてみてマージ
- 続いてshellcheck用のを別でdownloadShellcheckとして作ってマージ
- 共通化していく
- （オプション）download7zも共通化する。こちらはライセンスファイルも含む必要があってちょっと複雑
あるいは逆に全部共通化するプルリクを作り、レビューを通して良くしていくのもありだと思います。
ただこっちはかなり時間がかかるので、１つ１つちょっとずつ進める前者の方法がおすすめではあります。
とはいえ何事も経験だと思うので、好きな方で大丈夫です！！
腕に自身があれば後者でも、なければ前者が安定ではあります！
build/downloadTypos.js
Outdated
| /** | ||
| * Linuxに合わせたバイナリをダウンロードするための関数 | ||
| * @param {Object} params - バイナリの情報を含むオブジェクト | ||
| * @param {string} params.name - バイナリの名前 | ||
| * @param {string} params.url - 各プラットフォームのダウンロードURL | ||
| */ | ||
| async function downloadBinaryForLinux({ name, url }) { |
There was a problem hiding this comment.
ダウンロード→解凍→ファイル権限変更→削除の流れですが、たぶんlinux/mac/winで別々にせず、
nodejs fetch→7z解凍→nodjsのfs.chmod→unlinkSyncにすればほとんど共通化できると思います！
解凍だけ引数が違うので、getBinaryURLのように解凍用関数だけは作ることになるかもですが。
build/downloadTypos.js
Outdated
| // ダウンロードしたいバイナリデータの配列オブジェクト | ||
| const WANT_TO_DOWNLOAD_BINARIES = [ | ||
| { | ||
| name: "typos", | ||
| repo: "crate-ci/typos", | ||
| version: "latest", | ||
| }, | ||
| // { | ||
| // name: 'anotherBinary', | ||
| // repo: 'some-other-repo/some-binary', | ||
| // version: 'v1.2.3', // 指定したバージョン | ||
| // } | ||
| ]; |
There was a problem hiding this comment.
Githubリポジトリに沿う形で書くことで、他のを追加しやすくしてみた感じですよね！
VOICEVOXで長いことメンテしてて気づいたのですが、そもそもGithubでリリースしてない人とか、Githubに置いてるけどファイル名のOS・アーキテクチャの書き方がバラバラだったりとかいろいろあるので、逆に汎用性が下がっているかもです。
個人的には、数年単位でメンテすること考えると、ダウンロードURLを直書きしちゃうのが良い塩梅に感じてます。
（ @sevenc-nanashi さんと同じ意見になりそう）
修正します。
徐々に共通化したほうが失敗するリスクが低そうなため、前者がいいと思います。 |
ありがとうございます！！ お待ちしております･･･！ |
…ocディレクトリとREADME.mdを削除するスクリプトを追加した
There was a problem hiding this comment.
（判断メモです！）
こちらのファイルは他のファイルに比べるとコメントが結構多めで、一部冗長な箇所もあるなと感じました！
が、問題なさそうと思いました！
ここはビルドでしか使われなく、おそらく滅多に変更されることはないと思われます。
よく変更される箇所でしたらより洗練する方がいいかもしれませんが、ここはあまり変更されない場所なので、丁寧にコメントされているのはOKだと思います！
| const fileStream = createWriteStream(compressedFilePath); | ||
| await new Promise((resolve, reject) => { | ||
| response.body.pipe(fileStream); | ||
| response.body.on("error", reject); | ||
| fileStream.on("finish", resolve); | ||
| }); |
There was a problem hiding this comment.
（ただのコメントです）
ここ、たぶんパイプ経由で保存している感じですよね。
Node.js的にはもっとスマートの解決策があって、responseからバイナリーを得て、fs.writeFileなどで普通にファイル出力する方が一般的かもです。
もし次書く機会あればぜひ･･･！
内容
Dockerコンテナによる開発はしない方向のため、下記変更を取り込む
・Docker関連ファイルの削除
・上記に付随していたタイプミスを見つけ出すライブラリTyposのバイナリを使用したスクリプトの追加
・付随するドキュメントの整備
関連 Issue
スクリーンショット・動画など
その他
動作確認したOS