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

run.py: ルーティング関数の戻り値に型ヒントを追加 #764

Merged
merged 41 commits into from Oct 16, 2023

Conversation

aoirint
Copy link
Member

@aoirint aoirint commented Oct 9, 2023

内容

run.pyのルーティング関数(@app.getなどがついている関数)の戻り値に型ヒントを追加し、mypyのno-untyped-defエラーをルーティング関数に限って解消します。

誤ったオブジェクトを返すことによる不具合を型レベルで防げます。

変更量を少なくするため、ルーティング関数以外は変更していません。

合わせて変更した点

  • 変更した部分の周りの型ヒントを新しい記法に移行
    • Python 3.9で導入された記法: List[Type] -> list[Type]Tuple[Type] -> tuple[Type]
    • Python 3.10で導入された記法: Optional[Type] -> Type | None
  • /core_versionsの戻り値の型が実際にはResponseだがlist[str]で型ヒントがついていたのをResponseに修正
    • list[str]にする場合、Responseではないオブジェクトを返すように修正が必要
  • /installed_librariesのdocstringの戻り値の型の誤りを修正
  • setting_get(), setting_post()response_classHTMLResponseからResponseに変更
    • これらの関数ではstartlette.templating._TemplateResponse型が返るが、この型はHTMLResponseを継承しておらず、Responseを直接継承している
    • 先月HTMLResponseを継承する変更がマージされているが、まだリリースされていない
    • Responseを継承したオブジェクトを返すとresponse_classは無視される(FastAPIのソースコード@c1adce4)ので、実は設定する必要がない。そのままでもいいかもしれないけれど、戻り値の型との不一致は混乱するので避けたい

未修正な点(ルーティング関数内のmypy型エラー出力)

run.py:637: error: Incompatible return value type (got "HTTPException", expected "FileResponse")  [return-value]
run.py:770: error: Unsupported left operand type for / ("None")  [operator]
run.py:770: note: Left operand is of type "Optional[Path]"
run.py:774: error: Unsupported left operand type for / ("None")  [operator]
run.py:774: note: Left operand is of type "Optional[Path]"
run.py:781: error: Unsupported left operand type for / ("None")  [operator]
run.py:781: note: Left operand is of type "Optional[Path]"
run.py:785: error: Unsupported left operand type for / ("None")  [operator]
run.py:785: note: Left operand is of type "Optional[Path]"
run.py:795: error: Unsupported left operand type for / ("None")  [operator]
run.py:795: note: Left operand is of type "Optional[Path]"
run.py:950: error: Invalid type comment or annotation  [valid-type]
run.py:950: note: Suggestion: use conint[...] instead of conint(...)
run.py:992: error: Invalid type comment or annotation  [valid-type]
run.py:992: note: Suggestion: use conint[...] instead of conint(...)

関連 Issue

スクリーンショット・動画など

その他

mypyを実行

poetry run mypy .

@aoirint aoirint requested a review from a team as a code owner October 9, 2023 09:36
@aoirint aoirint requested review from Hiroshiba and removed request for a team October 9, 2023 09:36
@github-actions
Copy link

github-actions bot commented Oct 9, 2023

Coverage Result

Resultを開く
Name Stmts Miss Cover
run.py 446 298 coverage-33%
voicevox_engine/init.py 1 0 coverage-100%
voicevox_engine/acoustic_feature_extractor.py 75 0 coverage-100%
voicevox_engine/cancellable_engine.py 85 66 coverage-22%
voicevox_engine/dev/core/init.py 2 0 coverage-100%
voicevox_engine/dev/core/mock.py 27 12 coverage-56%
voicevox_engine/dev/synthesis_engine/init.py 2 0 coverage-100%
voicevox_engine/dev/synthesis_engine/mock.py 36 2 coverage-94%
voicevox_engine/downloadable_library.py 93 5 coverage-95%
voicevox_engine/engine_manifest/EngineManifest.py 34 0 coverage-100%
voicevox_engine/engine_manifest/EngineManifestLoader.py 12 0 coverage-100%
voicevox_engine/engine_manifest/init.py 3 0 coverage-100%
voicevox_engine/full_context_label.py 162 3 coverage-98%
voicevox_engine/kana_parser.py 86 1 coverage-99%
voicevox_engine/metas/Metas.py 33 0 coverage-100%
voicevox_engine/metas/MetasStore.py 29 12 coverage-59%
voicevox_engine/metas/init.py 2 0 coverage-100%
voicevox_engine/model.py 160 9 coverage-94%
voicevox_engine/mora_list.py 4 0 coverage-100%
voicevox_engine/morphing.py 70 46 coverage-34%
voicevox_engine/part_of_speech_data.py 5 0 coverage-100%
voicevox_engine/preset/Preset.py 12 0 coverage-100%
voicevox_engine/preset/PresetError.py 2 0 coverage-100%
voicevox_engine/preset/PresetManager.py 81 2 coverage-98%
voicevox_engine/preset/init.py 4 0 coverage-100%
voicevox_engine/setting/Setting.py 11 0 coverage-100%
voicevox_engine/setting/SettingLoader.py 19 0 coverage-100%
voicevox_engine/setting/init.py 3 0 coverage-100%
voicevox_engine/synthesis_engine/init.py 5 0 coverage-100%
voicevox_engine/synthesis_engine/core_wrapper.py 201 146 coverage-27%
voicevox_engine/synthesis_engine/make_synthesis_engines.py 59 30 coverage-49%
voicevox_engine/synthesis_engine/synthesis_engine.py 130 11 coverage-92%
voicevox_engine/synthesis_engine/synthesis_engine_base.py 67 9 coverage-87%
voicevox_engine/user_dict.py 144 11 coverage-92%
voicevox_engine/utility/init.py 5 0 coverage-100%
voicevox_engine/utility/connect_base64_waves.py 37 0 coverage-100%
voicevox_engine/utility/core_version_utility.py 8 1 coverage-88%
voicevox_engine/utility/mutex_utility.py 10 0 coverage-100%
voicevox_engine/utility/path_utility.py 26 8 coverage-69%
TOTAL 2191 672 coverage-69%

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です!!
細かい部分のコメントとても助かります!!

引数が1つの関数を改行するかどうかが結構ランダムに決められているように感じました。
まあ別に良いのですが、とりあえず既存のに合わせてできる限り1行にしておくと、配慮ができてちょっとかっこいいかもです!

run.py Show resolved Hide resolved
run.py Show resolved Hide resolved
run.py Outdated Show resolved Hide resolved
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!!!

調整ありがとうございます!
ず〜〜っと型ワーニングが出ていたの気になってたので嬉しいです!!!

@Hiroshiba Hiroshiba merged commit 0a26ebd into VOICEVOX:master Oct 16, 2023
3 checks passed
@aoirint aoirint deleted the patch-run_py_route_type_hint branch October 16, 2023 13:13
@aoirint aoirint mentioned this pull request Oct 17, 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.

None yet

2 participants