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

ユーザー辞書機能:単語の更新/削除機能を追加 #338

Merged
merged 11 commits into from
Feb 25, 2022

Conversation

takana-v
Copy link
Member

内容

題の通り

関連 Issue

その他

#334 (comment)
こちらの変更も一緒に行っています。
また、重複している処理があったので削除しています。(jsonへ2回書き込んでいた)

@github-actions
Copy link

github-actions bot commented Feb 21, 2022

Coverage Result

Resultを開く
Name Stmts Miss Cover
voicevox_engine/init.py 1 0 coverage-100%
voicevox_engine/acoustic_feature_extractor.py 75 0 coverage-100%
voicevox_engine/dev/synthesis_engine/init.py 2 0 coverage-100%
voicevox_engine/dev/synthesis_engine/mock.py 36 2 coverage-94%
voicevox_engine/full_context_label.py 162 3 coverage-98%
voicevox_engine/kana_parser.py 86 1 coverage-99%
voicevox_engine/model.py 125 7 coverage-94%
voicevox_engine/mora_list.py 4 0 coverage-100%
voicevox_engine/preset/Preset.py 12 0 coverage-100%
voicevox_engine/preset/PresetLoader.py 34 1 coverage-97%
voicevox_engine/preset/init.py 3 0 coverage-100%
voicevox_engine/synthesis_engine/init.py 5 0 coverage-100%
voicevox_engine/synthesis_engine/core_wrapper.py 156 126 coverage-19%
voicevox_engine/synthesis_engine/make_synthesis_engines.py 51 42 coverage-18%
voicevox_engine/synthesis_engine/synthesis_engine.py 108 2 coverage-98%
voicevox_engine/synthesis_engine/synthesis_engine_base.py 62 7 coverage-89%
voicevox_engine/user_dict.py 72 8 coverage-89%
voicevox_engine/utility/init.py 3 0 coverage-100%
voicevox_engine/utility/connect_base64_waves.py 35 3 coverage-91%
voicevox_engine/utility/engine_root.py 9 2 coverage-78%
TOTAL 1041 204 coverage-80%

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.

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

この辺りそろそろテストもあるとちょっと安心かもと思いました!
(今回のプルリクエストに必須ではないという認識です)

@aoirint さん、 @y-chan さん、レビューお願いできますか・・・?👀

voicevox_engine/user_dict.py Outdated Show resolved Hide resolved
@takana-v takana-v requested review from y-chan and aoirint February 24, 2022 09:19
Copy link
Member

@y-chan y-chan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

良さそうだと思いました!
一点だけ、テストに関して気になったので、コメントしておきます。

test/test_user_dict.py Outdated Show resolved Hide resolved
@takana-v takana-v requested a review from y-chan February 25, 2022 09:15
Copy link
Member

@y-chan y-chan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTMです!
これでエディタ側の実装にフェーズに移行できそうですね...!

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.

きれいに感じました!!

RESTっぽくするためにも、/idのとこだけ変更して頂けると・・・!

run.py Outdated Show resolved Hide resolved
run.py Outdated Show resolved Hide resolved
valid_dict_dict = {
"next_id": 1,
"words": {
"0": {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IDはintなんでしたっけ、stringなんでしたっけ

)

valid_dict_dict = {
"next_id": 1,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

将来辞書エディターみたないなのが出たりしたとき、next_idの数値と、word idがずれてしまう未来が見えました。
uuidにしても良いかも?

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!!!

初学者の方がPUTやDELETEの使い方をわからない可能性があるかもなので、READMEにcurlでの例があるとちょっとやさしいのかなと思いました。
(あっても間違っちゃうかもですが)

@Hiroshiba Hiroshiba merged commit 146b108 into VOICEVOX:master Feb 25, 2022
@takana-v takana-v deleted the add-rewrite-delete branch February 25, 2022 15:44
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.

3 participants