Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

整理: build_utiltools へリネーム #1388

Merged
merged 5 commits into from
Jun 19, 2024

Conversation

tarepan
Copy link
Contributor

@tarepan tarepan commented Jun 3, 2024

内容

build_util/tools/ scripts/ へリネームするリファクタリングを提案します。

関連 Issue

resolve #1380

@tarepan tarepan requested a review from a team as a code owner June 3, 2024 08:16
@tarepan tarepan requested review from Hiroshiba and removed request for a team June 3, 2024 08:16
@Hiroshiba
Copy link
Member

側にあるrun.specの扱い辺りどうしましょう 🙇
スクリプトっぽい感じではありますが、scriptsにあるのは若干の違和感がありそうかもです。

ちなみにspecはpyinstallerドキュメントによるとspecification(仕様)のことらしいです。

@tarepan
Copy link
Contributor Author

tarepan commented Jun 16, 2024

specはpyinstallerドキュメントによるとspecification(仕様)

👍️
同意です。
私も run.spec はその機能から「ビルド仕様書(build specification)」だと認識しています。

スクリプトっぽい感じではありますが、scriptsにあるのは若干の違和感

👍️
妥当な指摘です。
厳密な意味では直接呼ばれるスクリプトでないので、少しズレています。

「ビルドスクリプト ∈ scripts」であるとの前提で、「ビルドリソースはビルドスクリプトの仲間」と整理して scripts に入れてしまうのが簡潔だと感じています。
厳密にするなら script_resouces とか作るべきですが、そこを分ける恩恵があんまり無いかなと。

@Hiroshiba
Copy link
Member

なるほどです。
ビルドスクリプトがあれば、ビルドリソースがそこにあるのは納得感あるかもですが、無いですね…

toolsが丸い気がしてきました!

@tarepan tarepan changed the title 整理: build_utilscripts へリネーム 整理: build_utiltools へリネーム Jun 17, 2024
@tarepan
Copy link
Contributor Author

tarepan commented Jun 17, 2024

toolsが丸い

👍️
リネームします。


@Hiroshiba
全指摘箇所の反映・テストパスを確認しました。Re-review よろしくお願いします。

@Hiroshiba
Copy link
Member

あ、確認していませんでしたが、ちなみにこのディレクトリに入りそうな他のファイルって今のところありそうでしょうか?
あればそれも含められるように名付けをちょっと考えたいときたいなと…

@tarepan
Copy link
Contributor Author

tarepan commented Jun 18, 2024

現在の master において root 直下にあり、今後 tools/ へ移行する予定のファイル・ディレクトリは以下になります:

@Hiroshiba
Copy link
Member

おお・・・なるほどです。

pre-commitの方はconfigだと感じました。
toolsはどちらかというとこのプロジェクトを能動的に作用するためのツール群で、configは違う・・・・・かなぁ。。

run.specはギリギリで、本来buildコマンドがツールとしてあるけどそれがなく、そのビルドに使うためのファイルだけある、みたいな感じかなぁ。。
あとこれを深く議論するより、追加機能実装とかの議論を深めるほうがプロジェクトのためかもだし、ほんとに難しいですね。。。。

とりあえずtoolsへの改名は賛成です。
.pre-commit-config.yamlがtoolsに入らないことがなにか障壁になりそうなら、改名は待って、rootにある移動予定の全ファイルの整理をissue化して議論するのが進めやすいかもです。

@tarepan
Copy link
Contributor Author

tarepan commented Jun 19, 2024

#1408 にて root 直下ファイルの大規模移動は一旦見送りとなりました。
よって このディレクトリに入りそうな他のファイル は無くなりました。
レビューよろしくお願いします。

Copy link
Member

@Hiroshiba Hiroshiba left a comment

Choose a reason for hiding this comment

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

LGTM!!

少なくともbuild_utilは長い間実態に合ってなかったので、良かったです!!

@Hiroshiba Hiroshiba merged commit f26d124 into VOICEVOX:master Jun 19, 2024
4 checks passed
@tarepan tarepan deleted the refactor/scripts branch June 19, 2024 16:47
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.

refactor: build_util ディレクトリを scripts へリネーム
2 participants