-
Notifications
You must be signed in to change notification settings - Fork 246
refactor: tts_engine から song_engine に分離 #1592
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
Conversation
|
@tarepan さんもしよかったらまたレビューお願いさせていただいてもよろしいでしょうか・・・!! 🙇 ちなみにreviewerのアサインの通知とかってオンにされてたりしますか? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
ソング機能を SongEngine へ、共通関数を audio_postprocessing.py へ切り分けることで機能的凝集性が増しています。great job!
命名に関して、diff の見やすさという観点から、本 PR の段階ではこの命名で問題無いです(一部は既に _song_ が追加/削除されていますが誤差)。
命名およびディレクトリ分割は後続 issue/PR にて必ず取り組む必要があると考えます。以下は後続用のメモです:
TODO:
- ソング機能のテストをトーク機能のテストから分離
- ソング機能のテストに用いているSongEngineインスタンスの変数名が
tts_engineになっているのを修正 TalkInvalidInputError未使用の解消(トークでの適切なエラーハンドリング)- 命名とディレクトリ構造の再考
@Hiroshiba
はい、アサイン通知付けているので見えています。
レビュー手掛けられないケースも多いかもですが、「レビューしてくれたら嬉しいよ」通知として気軽にアサインして頂ければと思います。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!!
@tarepan さんのTODOコメントに同意です!
issue作成等どうするかお任せしてもよろしいでしょうか・・・? 🙇
tts_enginesはtalk_enginesとかでも良いかもですね!
sing / song / talk の使い分けはこんな感じかな~となっています・・・!
singとかsingingとかsongのなんとなくのルールの所感はこうかなと!
* 生成する系のAPIは動詞(sing・talk)
* sing_volume、sing_audio_query
* 物を指すときは名詞(song・talk)
* song_model、song_library、UI上の「ソング」
* 英語圏で一般的におかしいときはその限りではない
* singing_teacher、singing_synthesize| LatestVersion: TypeAlias = Literal["LATEST_VERSION"] | ||
| LATEST_VERSION: Final[LatestVersion] = "LATEST_VERSION" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
このあたりはTTS側と共通化できるかもですね!
| class SongEngineManager: | ||
| """Song エンジンの集まりを一括管理するマネージャー""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ここもEngineManagerとして統一できそう・・・かも?
Head branch was pushed to by a user without write access
内容
たたき台レベルなので、色々ツッコミよろしくお願いします。
例えば、クラス名は Song なのに、API は sing とか…
関連 Issue
close #1032