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

整理: UserDictionary クラスを追加 #1222

Merged
merged 1 commit into from
May 13, 2024

Conversation

tarepan
Copy link
Contributor

@tarepan tarepan commented May 10, 2024

内容

概要: UserDictionary クラスを追加してリファクタリング

VOICEVOX ENGINE はユーザー辞書の実装として read_dict() などの個別関数を多数定義し、それらを直接呼び出している。
これら関数が内部でアクセスする辞書は辞書パス引数によって設定されている。またデフォルトの辞書パスはハードコードしたうえでこのパス引数のデフォルト引数として渡されている。

その結果、現在のユーザー辞書 API は DI 不可能な構造となっている。これによりユーザー辞書 E2E テストの実装がブロックされている。
プリセットやマニフェストの実装はクラスベースとなっており、DI 可能な設計となっている。これを踏襲すれば DI 可能なユーザー辞書になる。

このような背景から、ユーザー辞書機能の UserDictionary クラス化によるリファクタリングを提案します。

関連 Issue

無し

Review 向け補足情報

UserDictionary クラス新設が主です。
UserDictionary は既存の read_dict() 等をラップし、従来関数呼び出しだった箇所をメソッド呼び出しで置き換えています。
既存の read_dict() 等はローカル関数になったため、_ prefix によるローカル化をしています。またこれらのデフォルト引数を廃止可能になったため、デフォルト引数は一括削除しています。

@tarepan tarepan requested a review from a team as a code owner May 10, 2024 14:41
@tarepan tarepan requested review from Hiroshiba and removed request for a team May 10, 2024 14:41
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.py Show resolved Hide resolved
test/user_dict/test_user_dict.py Show resolved Hide resolved
@Hiroshiba Hiroshiba merged commit c356918 into VOICEVOX:master May 13, 2024
3 checks passed
@tarepan tarepan deleted the refactor/user_dict_class branch May 14, 2024 03:43
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