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

整理: pre-commit 設定ファイルを移動 #1368

Closed
wants to merge 4 commits into from

Conversation

tarepan
Copy link
Contributor

@tarepan tarepan commented May 30, 2024

内容

pre-commit 設定ファイルをルートから build_util/ へ移動するリファクタリングを提案します。

git hook の更新後(pre-commit uninstall -t pre-push + pre-commit install -t pre-push --config build_util/.pre-commit-config.yaml)に git hook が想定通り機能することを確認済みです。

関連 Issue

無し

@tarepan tarepan requested a review from a team as a code owner May 30, 2024 16:06
@tarepan tarepan requested review from Hiroshiba and removed request for a team May 30, 2024 16:06
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・・・

なのですが、ちょっとrootディレクトリのファイルを片付けていって本当に良い感じになるのかちょっと不安になってきました。。。

というのも元は

pre-commit install -t pre-push

だったコマンドが

pre-commit install -t pre-push --config build_util/.pre-commit-config.yaml

になってるんですよね。。

そしてpre-commitがあるのかファイルからパッとわからなくなる・・・。
最終的なビジョンがどうなるか見通しが立っていないと、余計散らかるだけかもとちょっと思いました。。

@tarepan
Copy link
Contributor Author

tarepan commented Jun 3, 2024

rootディレクトリのファイルを片付けていって本当に良い感じになるのかちょっと不安
...
ファイルからパッとわからなく

👍️
一理あると思います。

Python 開発者のなかには pyproject.toml を嫌う人が一定数いて、その一因は「ルートの代わりに pyproject.toml が神ファイルになってるだけ。逆に見通し悪くなってない?」だと思います。それに対する反論は「pyproject.toml の全設定をルートにベタ書きしたら改善するとも思えない」ですかね。

今回もほぼ同じ議論かと思います。
「ルート直下に目的別ディレクトリを作り、そこにそれぞれ集約すべき」「ひと目見て全把握できるルートに置くべき」「本質的に複雑なんだから、割り切って各ツールのデフォルトに従うべき」どれも一長一短だと感じます。
私はひとまず 1 つめの方向性で PR たくさん出してる感じです。

一長一短なので、VOICEVOX ENGINE としての方針をえいやで決めるのが良さそうです。議論すれば理解は深まりますが、議論で唯一の正解が出ないタイプの問題だと思います。

@Hiroshiba
Copy link
Member

Hiroshiba commented Jun 10, 2024

一長一短になりえそうなこと、理解しました!

結構大きな変更になっていく気がしていて、仮にメリデメがトントンなら避けたほうが得かもと最近思い始めました。
既存コミッターがわからなくなってしまうためです。
もちろん変更後のほうが良いはずであれば変更したほうが良いのですが、どっちでも良いなら変更しないことが正義かもなと。

とはいえ、結構僕も散らかってるのは微妙な気がしていたのと、あとVOICEVOX全体のことを思うとちゃんと考えておくメリットがありそうだなと思ったので、いろいろ考え方を本腰入れて調べてみました!!


このnodejsのissueでの議論が興味深かったです。
あとはChatGPTくんと議論したり考えたりしてそこそこ以上の説得力がありそうなことを列挙してみます。

  • rootにあったほうが良いという考え
    • 全ファイルに影響を及ぼすファイルは、全ファイルのrootにあるべき
    • 対応していないものはrootに残り続ける
    • 新しく入った人が何使ってるか把握しやすい
    • サブディレクトリに分けるデファクトが無い
  • サブディレクトリにあったほうが良いという考え
    • rootに置くとスケールしないから
    • 重要なファイルが探しやすい
    • 他のプロジェクトに流用しやすい
    • 役割で細分化して管理できる
  • 他の考え方
    • プロジェクトファイル内で管理する
    • org全体として別管理する
    • GithubのUI上で.始まりのを非表示にできれば良い

個人的には「rootにおくとスケールしない」がroot離脱の一番強いモチベだと感じました。
何ファイルくらいから管理できなくなるのかわかりませんが、流石に今のENGINE rootのファイル数は多い。

一方で、ツール側が対応しておらず、役割ごとにまとめられないのであれば、散らばってしまう逆効果があると感じました。
こっちのツールはpyproject.toml、こっちは.configディレクトリ、こっちはroot、みたいになってくるのはちょっと良くない気もします。

VOICEVOXの都合で考えると、pyprojectに書いた場合他のリポジトリに流用しにくいというのはありそうです。
まあでもこれはわりと仕方なさそう。
あとコマンドが長くなってしまうのは・・・まあ仕方ないかな・・・。どっちにしろREADMEとかにコマンド見に行くので。
pyprojectのscripts内にショートカットコマンドを書いてpoetry run hogeで実行できるようにする、みたいな手もありそう。

なので結論としては、同じようなツールを全部pyprojectにまとめられるのか、あるいはサブディレクトリに配置できるのかがキーポイントになってきそうだなと思いました。
個人的には、綺麗にまとめてrootから押しのけられるなら、そうしてみたい気持ちはあります。
(とはみんな思いつつもやってるリポジトリは見たこと無いので、まあダメなんだろうなぁと薄々感じつつ・・・・・。)

@tarepan
Copy link
Contributor Author

tarepan commented Jun 13, 2024

VOICEVOX全体のことを思うとちゃんと考えておくメリットがありそうだなと思ったので、いろいろ考え方を本腰入れて調べてみました

👍️

個人的には「rootにおくとスケールしない」がroot離脱の一番強いモチベ

👍️
同意です。
ツラミの実体は「root に設定ファイルがあって美しくない」ではなく「root がゴチャついてて何がなんだかわからん」であり、このゴチャつき感とはスケールできていないことの現れだと感じます。

散らばってしまう逆効果があると感じました。
こっちのツールはpyproject.toml、こっちは.configディレクトリ、こっちはroot

👍️
同意です。
中途半端な移行はむしろ理解を阻害すると感じます。
幸いなことに、ENGINE の使うツール群は root からサブディレクトリへの全面移行が可能です(外部向けに値を表明している setup.cfg だけはたぶん動かせない(設定ファイルじゃないので root で無問題))。

同じようなツールを全部pyprojectにまとめられるのか

現状が既に「root か pyproject か」に割れています。pre-commit は root (.pre-commit-config.yaml)ですが black は pyproject([tool.black])です。
サブディレクトリ化しても「sub_dir か pyproject か」に変わるだけなので状況はほぼ変わりません。
「root か pyproject かに分かれているのがそもそも良くない」という指摘はありうると思いますが、サブディレクトリ移行とは別の議題であると感じます(pyproject.toml の存在意義を問うのと同義に見える)。

個人的には、綺麗にまとめてrootから押しのけられるなら、そうしてみたい気持ちはあります。

👍️
私の検証した範囲では root からサブディレクトリへ全面移行可能です。
移行したゆえのツラミは実際に移行してみないとわからないので、挑戦という位置づけで実装するのに賛成です。
実装とはいっても設定ファイル移動+コマンド変更なので、もし否定的な結論が得られても 1 PR でサクッと切り戻せます。

@Hiroshiba
Copy link
Member

Hiroshiba commented Jun 18, 2024

なるほどです!

現状が既に「root か pyproject か」に割れています。
サブディレクトリ化しても「sub_dir か pyproject か」に変わるだけなので状況はほぼ変わりません。

このPRの変更を取り込むと、「rootかsub_dirかpyprojectか」になっちゃいそうです。
一気にまとめてやってしまう方がプロジェクトの複雑度を挙げずに済み、中間状態を議論しなくて済むのでスムーズかもしれません。
最近、最終形がわからないまま中間状態に遷移するPRが多いイメージが有り、最終形の認識が無いまま進んじゃって、レビューしても中間状態だと判明して手戻りしたりが多いので、ちょっと意識したいです 🙏

まずはどのファイルをどこに持っていくのか、設計をissueで議論するのはどうでしょう?
逆にそれが揃えば移動は1PRで済む気がします。変更行数はプラマイ100もないのと、一貫してると思うので。

@tarepan
Copy link
Contributor Author

tarepan commented Jun 18, 2024

どのファイルをどこに持っていくのか、設計をissueで議論

👍️
現在移動が検討されているものの殆どが build_util (tools ) 行きなので、それが議論されている PR #1388 で最終形の認識合わせをできればと思います。

最終形がわからないまま中間状態に遷移するPRが多いイメージが有り、最終形の認識が無いまま進んじゃって、レビューしても中間状態だと判明して手戻りしたりが多い

👍️
レポジトリの全体最適を一時的に崩しそうな場合は最終形を issue でまず握る、ということですね。
方針理解しました、既存 PR に関してもいくつか issue が立つと思われます。

一気にまとめてやってしまう方がプロジェクトの複雑度を挙げずに済み、中間状態を議論しなくて済むのでスムーズ

これは私の経験則なのですが、OSS で複数機能を跨ぐデカ PR を作ると(内容は一貫していても)ツラミが勝ります。
最近の ENGINE だと #1184 が conflict 解決でかなり大変そうにみえました。
issue で最終形をまず握り、そのうえでパーツごとの PR をダダダッと出していく(一時的な中間状態は FIXME で目を瞑る)形が丸いのかなと思います。ただレポジトリのポリシーに拠ると思うので、中間状態を避けれる「issue 握り + デカ PR」の方が好ましければそちらの形で今後はコントリビュートします。

@Hiroshiba
Copy link
Member

Hiroshiba commented Jun 18, 2024

これは私の経験則なのですが、OSS で複数機能を跨ぐデカ PR を作ると(内容は一貫していても)ツラミが勝ります。
最近の ENGINE だと #1184 が conflict 解決でかなり大変そうにみえました。
issue で最終形をまず握り、そのうえでパーツごとの PR をダダダッと出していく(一時的な中間状態は FIXME で目を瞑る)形が丸いのかなと思います。ただレポジトリのポリシーに拠ると思うので、中間状態を避けれる「issue 握り + デカ PR」の方が好ましければそちらの形で今後はコントリビュートします。

おお、なるほどです。
たしかに大きめのPRの一部の議論が長期化して、その間にコンフリクトが重なっていくことがありそうです。

最初にゴール感はあるとレビューが捗るのであったほうが良さそう。
そこからの変更の粒度はわりとお任せで、自分の経験則側上変更行数が+-合計100〜200行ならレビューは大変じゃない、+-合計300〜400も意外といける、けど大きな変更はコンフリクト対応が必要になってくるので中間状態が生まれてしまうのは仕方ない、移動は最初にあると分割数を下げれそう(最後でも良いけどたぶん途中で移動したくなる)・・・みたいな感じかもです。

途中なコードになってしまうのは仕方ないことだと感じました、ありがとうございます!!

@tarepan
Copy link
Contributor Author

tarepan commented Jun 19, 2024

#1408 にて root 直下ファイルの大規模移動は一旦見送りとなった。
本 PR は NoGo につき close とします。

@tarepan tarepan closed this Jun 19, 2024
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.

2 participants