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

[vvm-async-api] Add: ユーザー辞書APIを追加 #538

Merged
merged 161 commits into from
Jul 22, 2023

Conversation

sevenc-nanashi
Copy link
Member

内容

ユーザー辞書を実装します。

関連 Issue

その他

C APIの設計は初めてなので指摘はどんどんしていただけると

@sevenc-nanashi sevenc-nanashi changed the title Add: ユーザー辞書を追加 Add: ユーザー辞書APIを追加 Jul 8, 2023
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.

draft PR良いですね!
API設計で認識違いそうなとことかが今のうちから話せる。



@pydantic.dataclasses.dataclass
class UserDictWord:
Copy link
Member

Choose a reason for hiding this comment

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

ここ、Rust実装のバリデータを隠しAPIを通したりしてここに持ってきて、Pydanticとしてバリデートされるようにした方がいいかも。

(面倒だろうし、やるとしても今でなくてもいいと思います)

Copy link
Member Author

Choose a reason for hiding this comment

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

diffがとんでもないことになってるので別のPRで諸々(例えばPython APIのlib.rsの分割とか)と一緒にやろうと思います。

@sevenc-nanashi
Copy link
Member Author

レビューを反映しました。

Copy link
Member

@qryxip qryxip left a comment

Choose a reason for hiding this comment

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

LGTM!!!

お疲れ様です。よい設計だと思います。

Rustの書き方が結構身に付いてきているかと思います。

@qryxip
Copy link
Member

qryxip commented Jul 20, 2023

@Hiroshiba #497 の方、YちゃんさんとSuitCaseさんからOKをもらってますし、あとはタスクリストの移行ができればマージできるかなと思っているのですが、マージの順番どうしましょう? このPRが先?

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

お疲れ様でした!!
もしよかったら今回の経験の感想とかを一言でもいいのでDiscord等で語ってください!
メンテナとしても学びがあると思うし、他の方の勢いや @sevenc-nanashi さんの評価にもなるかなと思うので・・・!

@Hiroshiba
Copy link
Member

@qryxip マージの順序は先に vvm-async → main を済ませると良さそうです。
YちゃんさんとSuitCaseさんからのOKは今のvvm-asyncに対してなので!

そのあとmainに辞書PRをマージすることに関しては僕たち2人のapproveで良いかなと。
という流れでどうでしょう 👀

@qryxip
Copy link
Member

qryxip commented Jul 21, 2023

はい。それがいいと思います。

@qryxip qryxip linked an issue Jul 21, 2023 that may be closed by this pull request
@qryxip
Copy link
Member

qryxip commented Jul 21, 2023

ところで #265 がちゃんとlinkされてなかったみたいなので、今しました。

@sevenc-nanashi sevenc-nanashi changed the base branch from project-vvm-async-api to main July 22, 2023 13:58
@sevenc-nanashi
Copy link
Member Author

宛先を変更しました。

@Hiroshiba
Copy link
Member

プロジェクトブランチがマージされたのでこちらもマージしたいと思います!!

このコメントが残っているので必要であればissue化するかタスクリストに追加してもいいかもです!
#538 (comment)

@Hiroshiba Hiroshiba merged commit 8cf307d into VOICEVOX:main Jul 22, 2023
58 checks passed
@sevenc-nanashi
Copy link
Member Author

このコメントが残っているので必要であればissue化するかタスクリストに追加してもいいかもです!

これは今からPR出そうと思います。

@Hiroshiba
Copy link
Member

そういえばツイートしていなかったのでツイートしました!!
https://twitter.com/voicevox_pj/status/1682875752738615296

Hiroshiba pushed a commit that referenced this pull request Jul 28, 2023
* Refactor: Python APIの変換系を移動

* Add: pydanticにバリデーターを追加

* Add: pyiにVoicevoxErrorを記述

Co-Authored-By: Qryxip <[email protected]>

* Add: to_zenkakuを通すように

* Update crates/voicevox_core_python_api/python/voicevox_core/_rust.pyi

Co-authored-by: Ryo Yamashita <[email protected]>

---------

Co-authored-by: Qryxip <[email protected]>
Co-authored-by: Ryo Yamashita <[email protected]>
@qryxip qryxip mentioned this pull request Aug 16, 2023
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.

user定義辞書の作成・読み込み機能を作る
4 participants