VOICEVOX / voicevox_core

無料で使える中品質なテキスト読み上げソフトウェア、VOICEVOXのコア
https://voicevox.hiroshiba.jp/
MIT License
855 stars 115 forks source link

derive-gettersをamplify_deriveに入れ替える #460

Open qryxip opened 1 year ago

qryxip commented 1 year ago

内容

voicevox_coreでgetterを生成するたに使われているderive-gettersamplify (amplify_derive)に入れ替えます。このリポジトリには現在PRが溜まりまくっているため、今はissueを作るのみに留めます。

derive-gettersには制限があって限られた形の生成しかできませんし、おそらく新機能の導入も今後無いでしょう。例えばStringのフィールドを&strとして出すことができませんし、デフォルト値の設定もできません (#214) (こっちはderive-newの話でした。下で #214 の話を入れた時に混じった)。 あと&mutのgetterも作れません。 (追記) 「Copyなやつも問答無用で(&self) -> &Tの形になる」を入れ忘れてました。これが一番大きい。

What this crate won't do

There are no mutable getters and it's not planned. There are no setters either nor will there ever be.

amplifyであれば慣習的な形のgetterを生成できると思います。将来Rust APIを提供できるかもしれないことを考えると、検討しても良いかと思います。

またamplifyは現在、少なくともderive-gettersよりは活発に開発されているように見えます。これだけでも理由になるかと。

Pros 良くなる点

Cons 悪くなる点

なし

実現方法

-use derive_getters::Getters;
+use amplify::Getters;

VOICEVOXのバージョン

0.15.*

OSの種類/ディストリ/バージョン

その他

そもそもRustでは他言語ほどgetterを執拗に作る必要は無いのではと思っています。C-STRUCT-PRIVATEもあらゆるfieldの露出をgetter/setterにしろとは言ってないはず。例えばこれには流石に要らないと感じています (将来的にフィールドを追加するのであれば#[non_exhaustive]でも付けておけばよい)。

https://github.com/VOICEVOX/voicevox_core/blob/0049bc51775c153fe513fa8ee2ba2cbe00e2c415/crates/voicevox_core/src/status.rs#L168-L173

あと話が少々ずれるかもしれませんが、別の場所で「Rustはbatteries excludedだ」的なことを言ったりしてたのですが、表現したいことを歪められてしまう場合に限ってはコード量が膨れてでも「標準的な」書き方を優先するべきだとも思っています。(derive-newの方の話ですが)今考えると #214 のときに #220 を出すんじゃなくてそういうことを言うべきだったと思ってます。

qryxip commented 1 year ago

表現したいことを歪められてしまう場合に限ってはコード量が膨れてでも「標準的な」書き方を優先するべきだとも思っています。

いや冷静に考えるとこれも程度問題ですね…

例えばserde::Serializeが常にfallibleであるせいでこういうunwrap/expectが必要になるからといって、JSONへの変換はserdeじゃなくてminiserdenanoserdeでやろうと言われたら渋い顔になると思います。流石にserdeの歴史と安定感を考えると、std::path → caminoのようには…

https://github.com/VOICEVOX/voicevox_core/blob/0049bc51775c153fe513fa8ee2ba2cbe00e2c415/crates/voicevox_core/src/status.rs#L197-L199

(ただ実際に来た場合、ここ(VOICEVOX全体)のメンバーからの提案だったら懸念を伝えるのみ、新規のfirst-time contributorからだったりしたらそのままOK出してしまう、という風にしてしまうかも) (追記) これ違う話だ。https://github.com/VOICEVOX/open_jtalk-rs/pull/12#issuecomment-1499490739に移しました

voicevox_core内での具体例だと他に出てこない... まあ関数内での定義だったり、pub(crate)だったりの場合はAPIが不恰好でもいいかなと。あと挙動が望ましくないから手作りしようという時、その挙動がどれくらい重大かとか実装コスト/メンテコストがどれだけかかるかとの相談になるかと。

Hiroshiba commented 1 year ago

issue作成ありがとうございます!

大前提として、どっちでも良いだろうなという感じです。 というのも現状困っておらず、将来的にも問題になるとは言い切れず、かつVOICEVOXのメインロジックに直接関与していないためです。 将来問題になるかもしれない点を、今解消できるかもって感じでしょうか。

という前提はありますが、せっかくなので両方のライブラリを眺めさせていただきました。 導入判断ですが、正直なところderive-gettersのほうがメンテナとしては良いかもと思いました。 (依存している人の数が桁違いだったので) https://crates.io/crates/amplify https://crates.io/crates/derive-getters

ただコメントを読んで、課題になる点もあってくるんだろうなと思いました。 今回のissueは「issueとしてしっかりまとめてくださった」というところが大きな貢献になっているのかなと思いました。 なので今回はこの提案があったところで完了で、将来課題が発生したときにこのissueをreopenして速攻で対処する、とかどうでしょう?

Hiroshiba commented 1 year ago

せっかくなのでゴール感のすり合わせができればと思って追加でコメントしてみます。

VOICEVOXが思う目標は↓にあります。 https://github.com/VOICEVOX/voicevox/blob/876d5193d97228062ae3ddc0439ff9518b529789/docs/%E3%83%9F%E3%83%83%E3%82%B7%E3%83%A7%E3%83%B3%E3%83%BB%E3%83%90%E3%83%AA%E3%83%A5%E3%83%BC%E3%83%BB%E3%83%93%E3%82%B8%E3%83%A7%E3%83%B3.md

ここに関与することは割りと積極的に改善していきたいのですが、そうじゃない点に時間を書けているとこれらの点の改善が遅れてしまうので、どうしても後回しになってしまいます。 例えば今回の古いライブラリのパージについて、そもそもgetters部分がVOICEVOXの基幹に関与しておらず、バリューのユーザー数・技術力・新規性にも関与しないため、判断に熱量をかけられない(時間をかけられない)という感じです。

なので提案は嬉しいし意味のあることだとは思うのですが、熱量がずれちゃっててもったいないなーと。 お互い情熱が注げるポイントがあれば最高なので、そこを模索できればお互いハッピーなのかなと思いました。 例えばこれとかどうでしょう。

qryxip commented 1 month ago

https://github.com/VOICEVOX/voicevox_core/pull/807#discussion_r1672275410