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

実装:MinimumEngineManifest格納時の検証をZodで行う #1186

Merged
merged 10 commits into from
Feb 10, 2023

Conversation

nmori
Copy link
Contributor

@nmori nmori commented Feb 7, 2023

内容

・エンジン追加時および起動時のminimumEngineManifestを Zodで検証してから格納する
・検証に当たり、portの型が異なるようにみえたので修正(string→number)

関連 Issue

ref #1181

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

その他

 

@nmori nmori requested a review from a team as a code owner February 7, 2023 15:49
@nmori nmori requested review from y-chan and removed request for a team February 7, 2023 15:49
src/type/preload.ts Outdated Show resolved Hide resolved
src/type/preload.ts Outdated Show resolved Hide resolved
src/type/preload.ts Outdated Show resolved Hide resolved
};

export const minimumEngineManifest = z
Copy link
Member

Choose a reason for hiding this comment

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

ここもSchemaだと統一感あるかもです

Suggested change
export const minimumEngineManifest = z
export const minimumEngineManifestSchema = z

Copy link
Member

Choose a reason for hiding this comment

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

あ、ここ忘れられてるかもです!

src/type/preload.ts 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です!!

MinimumEngineManifestにしてるところって結構あったんですね。

@nmori
Copy link
Contributor Author

nmori commented Feb 7, 2023

ありがとうございます!
レビュー頂いた部分、納得でしたので修正PRをお送りします。
(お時間頂き恐縮です)

src/type/preload.ts Outdated Show resolved Hide resolved
@@ -1,6 +1,7 @@
import { IpcRenderer, IpcRendererEvent, nativeTheme } from "electron";
import { IpcSOData } from "./ipc";
import { z } from "zod";
import { EngineManifest } from "@/openapi";
Copy link
Member

Choose a reason for hiding this comment

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

ここ'EngineManifest' is defined but never usedと出てたので消しちゃっても良さそうです!

Suggested change
import { EngineManifest } from "@/openapi";

@Hiroshiba
Copy link
Member

こちらのコメントどうでしょう 👀 (重要ではないのでそのままでも大丈夫です!)
#1186 (comment)

あとコンフリクトが発生していそうです 🙇‍♂️
分からない点あれば聞いて頂ければ・・・!

  * 命名規則の適用(スキーマ)
  * 不要な参照の削除
* src/background/engineManager.ts
* src/background/vvppManager.ts
* src/type/preload.ts
* src/background/engineManager.ts
* src/background/vvppManager.ts
* src/type/preload.ts
@nmori
Copy link
Contributor Author

nmori commented Feb 9, 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.

LGTM!!

VOICEVOXのgitログは全部squashされるので大丈夫です!!

src/type/preload.ts Outdated Show resolved Hide resolved
@Hiroshiba
Copy link
Member

問題ないと思うのでマージします!!

@Hiroshiba Hiroshiba merged commit 3fab509 into VOICEVOX:main Feb 10, 2023
@nmori nmori deleted the Branch_4d3e408c branch February 19, 2023 07: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.

2 participants