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

整理: 内部型 WordProperty を追加 #1333

Merged
merged 8 commits into from
Jun 2, 2024

Conversation

tarepan
Copy link
Contributor

@tarepan tarepan commented May 26, 2024

内容

概要: 内部型 SimpleUserDictWord を追加してリファクタリング

VIOCEVOX ENGINE のユーザー辞書機能には 2 種類のワード概念がある。
一方は OpenJTalk の単語であり、UserDictWord で表現される。このクラスは辞書のインポート/エクスポートで API に登場する。
他方は VOICEVOX の簡易単語であり、属性の集合(surface / pronunciation / accent_type / word_type / priority)で表現される。これら属性は単語 API で利用され、ENGINE 内部で UserDictWord へ変換されている。

後者の属性群は意味論的にひとかたまりであるにも関わらず、クラスが用意されていないためバラバラの引数・変数として内部で引き回されている。
その結果、コードの見通しに改善の余地がある。

このような背景から、内部型 SimpleUserDictWord を追加するリファクタリングを提案します。

Note for Reviewer

#1242 でのテスト有効化後の方がより安全

関連 Issue

ref #1330

@tarepan tarepan requested a review from a team as a code owner May 26, 2024 09:42
@tarepan tarepan requested review from Hiroshiba and removed request for a team May 26, 2024 09:42
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です!!

ちょっと混乱起きそうな気配を感じたので、ドキュメントコメントだけお願いさせていただきました 🙇

SimpleUserDictWordとUserDictWord、どっちが外に出るやつなのかパッとわからなくて混乱するかもですね・・・。
(特にUserDictWordがどっちかわからない)

うーん。ちょっとベストプラクティスがわからない・・・。

voicevox_engine/user_dict/user_dict.py Outdated Show resolved Hide resolved
voicevox_engine/user_dict/user_dict.py Outdated Show resolved Hide resolved
@tarepan
Copy link
Contributor Author

tarepan commented May 29, 2024

SimpleUserDictWordとUserDictWord、どっちが外に出るやつなのかパッとわからなくて混乱するかも

👍️
同意です。
とはいえ、API に事実上 2 種類の単語(SimpleUserDictWord相当の引数集合、UserDictWord)が用意されてしまっているので、本質的には API 設計レベルでしか直せなそうです。

あくまで緩和策ですが、前者を「単語属性のあつまり」と捉え直して WordProperty にリネームしてみました。
これで「引数群を WordProperty クラスに詰める」「WordProperty から UserDictWord を作る」といった具合にわかりやすくできそうと考えました。


@Hiroshiba
全指摘箇所の反映・テストパスを確認しました。Re-review よろしくお願いします。

@tarepan tarepan changed the title 整理: 内部型 SimpleUserDictWord を追加 整理: 内部型 WordProperty を追加 May 29, 2024
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!!

voicevox_engine/user_dict/user_dict_word.py Outdated Show resolved Hide resolved
@Hiroshiba Hiroshiba merged commit 4e13143 into VOICEVOX:master Jun 2, 2024
4 checks passed
@tarepan tarepan deleted the refactor/internal_simple_word branch June 2, 2024 16:37
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