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

Synthesizerの構造改革をする #685

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

qryxip
Copy link
Member

@qryxip qryxip commented Nov 18, 2023

内容

Synthesizserの改革を行います。

#553 (comment)

  1. SynthesisEngineを解体し、Synthesizser内に
  2. SynthesizserSynthesizser<impl Borrow<OpenJTalk> + ...> | Synthesizser<()>
  3. APIの浅い所 (compatible_engineに至るまで)にndarrayを要求

関連 Issue

#545

その他

@Hiroshiba
Copy link
Member

PRありがとうございます!

1600行の更新、なるほどです。
おそらくコードの移動が大半になっているので、そこだけでもプルリクエストを分離していただけると助かるかもしれません。
移動だけなら差分をレビューすることが簡単なのですが、移動に変更が加わっていると難度が跳ね上がってしまうので・・・

ただまあ、今回は仕方ないということにして、APIの設計があまり変わっていないのであれば、ほぼレビューなしでマージすることも可能かもしれません。
Googleのエンジニアプラクティスに、変更は小さくしてプルリクを投げた方が良いという思想があります。
コミッター・レビュワー双方にメリットがあるというロジックなのでよかった読んでみてください!
https://google-engineering-practices.translation.shuuji3.xyz/review/developer/small-cls.html

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