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

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

Open
3 tasks done
qryxip opened this issue Apr 5, 2023 · 4 comments
Open
3 tasks done

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

qryxip opened this issue Apr 5, 2023 · 4 comments

Comments

@qryxip
Copy link
Member

qryxip commented Apr 5, 2023

内容

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 良くなる点

  • より慣習的な形のgetterにできる
  • よりメンテナンスが活発なライブラリに依存できる

Cons 悪くなる点

なし

実現方法

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

VOICEVOXのバージョン

0.15.*

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

  • Windows
  • macOS
  • Linux

その他

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

#[derive(Getters, Debug, Serialize, Deserialize)]
pub struct SupportedDevices {
cpu: bool,
cuda: bool,
dml: bool,
}

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

@qryxip
Copy link
Member Author

qryxip commented Apr 6, 2023

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

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

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

pub fn to_json(&self) -> serde_json::Value {
serde_json::to_value(self).expect("should not fail")
}

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

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

@qryxip qryxip mentioned this issue Apr 7, 2023
3 tasks
@Hiroshiba
Copy link
Member

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

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

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

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

@Hiroshiba
Copy link
Member

Hiroshiba commented Apr 8, 2023

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

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
Copy link
Member Author

qryxip commented Jul 10, 2024

#807 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants