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

feat! [Python] リテラル型をnon exhaustiveに #957

Merged

Conversation

qryxip
Copy link
Member

@qryxip qryxip commented Jan 29, 2025

内容

#941, #955, #950 の続き。

_Reservedvoicevox_core._models._Reserved voicevox_core._models._please_do_not_use._Reserved としてでしか存在せず、ユーザーは原則触ることはできない状態にしてある。

不正な値を入れようとしたときのPydanticのエラーはこうなる。

python
Python 3.10.16 (main, Jan  1 2025, 22:31:19) [GCC 14.2.1 20240910] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> from voicevox_core import UserDictWord
>>> UserDictWord("ア", "ア", word_type="不正な値")
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/ryo/.cache/pypoetry/virtualenvs/voicevox-core-hMLxFPJX-py3.10/lib/python3.10/site-packages/pydantic/_internal/_dataclasses.py", line 141, in __init__
    s.__pydantic_validator__.validate_python(ArgsKwargs(args, kwargs), self_instance=s)
pydantic_core._pydantic_core.ValidationError: 2 validation errors for UserDictWord
word_type.literal['PROPER_NOUN','COMMON_NOUN','VERB','ADJECTIVE','SUFFIX']
  Input should be 'PROPER_NOUN', 'COMMON_NOUN', 'VERB', 'ADJECTIVE' or 'SUFFIX' [type=literal_error, input_value='不正な値', input_type=str]
    For further information visit https://errors.pydantic.dev/2.9/v/literal_error
word_type.function-after[_no_input_allowed(), any]
  Value error, No input is allowed for `_Reserved` [type=value_error, input_value='不正な値', input_type=str]
    For further information visit https://errors.pydantic.dev/2.9/v/value_error

またドキュメントはこんな感じ。_Reservedにリンクが張られていないのは、上記の通り意図したもの。

image

関連 Issue

その他

@qryxip qryxip requested a review from Hiroshiba January 29, 2025 18:25
@Hiroshiba Hiroshiba requested a review from Copilot January 30, 2025 02:31

Choose a reason for hiding this comment

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

Copilot reviewed 1 out of 1 changed files in this pull request and generated no comments.

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!

結構pydanticの機能に依存している箇所があると思うので、pydanticのバージョンをあげたときもちゃんと動作していることを確認するテストを追加しても良いのかもなーと感じました!
(ちょっとちゃんと把握してないのでどういうテストがいるのか、そもそも必要なのかわかってませんが・・・)

不要だったらマージ、あったほうが良ければ実装&再レビュー、もしくはissue化とかで良さそう!

@qryxip
Copy link
Member Author

qryxip commented Jan 30, 2025

337d2ab (#957): voicevox_core._models._Reservedを、voicevox_core._models._please_do_not_use._Reservedに移動しました。

@qryxip qryxip force-pushed the pr/feat-python-make-literal-types-non-exhaustive branch from 814db3a to 337d2ab Compare January 30, 2025 02:55
@qryxip
Copy link
Member Author

qryxip commented Jan 30, 2025

結構pydanticの機能に依存している箇所があると思うので、pydanticのバージョンをあげたときもちゃんと動作していることを確認するテストを追加しても良いのかもなーと感じました! (ちょっとちゃんと把握してないのでどういうテストがいるのか、そもそも必要なのかわかってませんが・・・)

不要だったらマージ、あったほうが良ければ実装&再レビュー、もしくはissue化とかで良さそう!

ちょっと理解しきれていないのですが、正常系は既にいっぱいパスが通っていることを考えるとエラー周りですかね?例えば不正な文字列を入れたら変な例外ではなくちゃんとpydantic.ValidationErrorで返されることとか…?

例:

with pytest.raises(pydantic.ValidationError):
dict_a.add_word(
voicevox_core.UserDictWord(
surface="",
pronunciation="カタカナ以外の文字",
)
)

@qryxip
Copy link
Member Author

qryxip commented Jan 30, 2025

3c43148 (#957): 異常系のテストを一つ書いてみました。

@Hiroshiba
Copy link
Member

Hiroshiba commented Jan 30, 2025

あ、ですです!
バリデーション等が正しく動作するかのチェックが無く、動かなくなっていても気づかなそうという意図でした。

あるとしたら追加された想定外の文字が渡されたパターンと、あと_Proposedがpydanticの中で正しく動くか(=__get_pydantic_core_schema__を通る経路が正しく動くか?)とかかなと思ってました。
後者は不要なのかも。

@qryxip
Copy link
Member Author

qryxip commented Jan 30, 2025

なるほどです。であればさっき追加したテストで両方カバーできているかなと思いました。

@qryxip qryxip merged commit 78a92a9 into VOICEVOX:main Jan 30, 2025
30 checks passed
@qryxip qryxip deleted the pr/feat-python-make-literal-types-non-exhaustive branch January 30, 2025 06:24
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