Skip to content

Conversation

@Hiroshiba
Copy link
Member

内容

の解決PRです!

関連 Issue

fix #1577

その他

判断基準の詳しくはこちらへ

@Hiroshiba Hiroshiba requested a review from a team as a code owner March 31, 2025 06:44
Comment on lines -19 to 7
python_version = 3.11
show_error_codes = True
strict_equality = True
strict_optional = True
warn_redundant_casts = True
warn_return_any = True
ignore_missing_imports = True
warn_unreachable = True
Copy link
Member Author

Choose a reason for hiding this comment

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

残った2つの僕としての気持ちこんな感じです:

  • ignore_missing_imports
    • いずれオンにしたい気持ちはあるけど、恩恵と辛さを考えると難しいかも
  • warn_unreachable
    • 非到達かどうかは型チェックをしないとわからないこともあるのでmypyが担っているのは自然かも
    • 個人的にはデッドコードに気づけるきもするので、オンにしておきたい気持ちが少しあります

Copy link
Contributor

Choose a reason for hiding this comment

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

PRありがとうございます!
issueのほうでもいろいろ調べて比較してくださりありがとうございました。
pysenのstrictとの相違点については私も同様の認識です。

ignore_missing_imports については、あまり調べてないですが確かにちょっと難しそうなので一旦Trueでよさそうです。
warn_unreachable についても、確かに型チェックの結果分かるというシーンもありそうだからやったほうがよさそうですね…。仰る通りです。

なので特に問題無さそうだと思いました!

Copy link
Member Author

Choose a reason for hiding this comment

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

見ていただけて助かります、ありがとうございます!!

@Hiroshiba Hiroshiba requested review from Copilot and tarepan March 31, 2025 06:45
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR updates various type annotations and import groupings to support stricter mypy settings. Key changes include updating type hints for JSON and list/dictionary types, reordering and grouping imports, and incorporating NDArray type usage.

Reviewed Changes

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

Show a summary per file
File Description
voicevox_engine/app/routers/user_dict.py Reorganized and grouped imports from the user dictionary module.
tools/merge_engine_manifest.py Improved dict type annotations for JSON merging functionality.
test/utility.py Added NDArray type annotations to better reflect numpy array usage.
test/unit/user_dict/test_user_dict.py Reorganized imports to align with the updated mypy strict configuration.
test/unit/tts_pipeline/test_tts_engine.py Updated list type annotations for clearer intent with AccentPhrase.
test/e2e/test_disable_api.py Introduced more explicit type hints for app parameters.
test/e2e/single_api/tts_pipeline/test_cancellable_synthesis.py Enhanced fixture typing with dict[str, Any] for improved type safety.
test/e2e/conftest.py Updated fixture parameter types for consistency with mypy requirements.
Files not reviewed (1)
  • mypy.ini: Language not supported

@Hiroshiba
Copy link
Member Author

もしよかったら @nanae772 さんもぜひコメントいただけると・・・!!!

Copy link
Collaborator

@tarepan tarepan left a comment

Choose a reason for hiding this comment

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

LGTM!

厳密な型チェックの導入により、より安全なコードを、より開発の初期段階から書けるようになっています。good work!

@tarepan tarepan enabled auto-merge April 10, 2025 13:56
@Hiroshiba
Copy link
Member Author

レビューありがとうございます!! マージします!!

@Hiroshiba Hiroshiba disabled auto-merge April 11, 2025 06:48
@Hiroshiba Hiroshiba enabled auto-merge April 11, 2025 06:48
@Hiroshiba Hiroshiba added this pull request to the merge queue Apr 11, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Apr 11, 2025
@nanae772
Copy link
Contributor

すみません、#1596 のリファクタリングをしたときに私がつけた型が根本的に間違ってて

def generate_licenses_from_licenses_json(licenses_json: dict) -> list[License]:

ここは

def generate_licenses_from_licenses_json(
    licenses_json: list[dict[str, str]],
) -> list[License]:

でした。お手数ですが直していただければマージできるようになると思います🙇‍♂️

@tarepan
Copy link
Collaborator

tarepan commented Apr 11, 2025

#1596 のリファクタリングをしたときに私がつけた型が根本的に間違ってて

確認しました。こちらこそレビューで見逃していました、原因究明ありがとうございます。
hotfix PR を作成しました(#1602)。これをマージすれば問題解決するはずです。

@Hiroshiba Hiroshiba enabled auto-merge April 11, 2025 12:46
@Hiroshiba
Copy link
Member Author

お二人ともありがとうございます!! 助かります 🙇

@Hiroshiba Hiroshiba added this pull request to the merge queue Apr 11, 2025
Merged via the queue into VOICEVOX:master with commit 0933459 Apr 11, 2025
5 checks passed
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.

mypyのstrictモードを使う

3 participants