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で行う #1181

Closed
3 tasks
nmori opened this issue Feb 5, 2023 · 5 comments
Closed
3 tasks

MinimumEngineManifest格納時の検証を Zodで行う #1181

nmori opened this issue Feb 5, 2023 · 5 comments

Comments

@nmori
Copy link
Contributor

nmori commented Feb 5, 2023

内容

JSONデータをそのままパース・代入している点について
検証をおこなうべきという潜在的課題について解決を行う ref #1179

Pros 良くなる点

・変数内の状態が保証される

Cons 悪くなる点

・特になし?

実現方法

VOICEVOXのバージョン

OSの種類/ディストリ/バージョン

  • Windows
  • macOS
  • Linux

その他

@nmori
Copy link
Contributor Author

nmori commented Feb 5, 2023

現段階でなんとなく動くところまで 書くことができたのですが
1点、どう実装していいか分からない部分があります

export const minimumEngineManifest = z.object({
  name: z.string().default(""),
  uuid: z.string().default(""),
  command: z.string().default(""),
  port: z.string().default(""),
  supported_features: z.record(z.string(), z.boolean()),
});

supported_featuresは、文字列とbooleanの組み合わせという評価でよいか、
定義済みの構造に沿っているかをみるべきか?という点です。

後者の場合はクラスを指定するのかな?とおもいますが
ちょっとどのように表記してよいのかわかりません
(現段階での私の技量を超えていて、コードの表現方法が思いつかないです)

@Hiroshiba
Copy link
Member

Hiroshiba commented Feb 6, 2023

issue作成ありがとうございます!!

zodで使うには実態(文字列とか)が無いといけないのですが、定義済みなのは型(Type)なのでおそらく使えないと思います!
ちょっとむずいのですが、TypeScriptはts→jsにトランスパイルしていて、前者はjsの概念ですが、後者はtsだけにある概念です。
今回の場合SupportedFeaturesとして存在するのは型だけなので、ここではz.record(z.string(), z.boolean())とするしか無いのかなと!

欲を言うとsupported_featuresはcamelCaseにしてsupportedFeaturesとしたいですね!

@nmori
Copy link
Contributor Author

nmori commented Feb 7, 2023

すこしZodを勉強してみました。
手元のテスト段階では、おおよそ期待通りに動くように出来たと思うので
PR出したいと思います。

なお、変数名がJSONラベル名と一致していないと解析時にエラーとなったので
最小限の範囲でcamelCaseを適用せずに記述しました。

ちなみに、経験不足ゆえ、こういう場合のPRの出し方について作法を1つご教示頂きたく。

今回のは ref #1179 に関連するかたちですが課題が異なると思ってIssueをわけました。
しかし、1つを進行中で 関連するもう1つをPRする場合は、ローカルでブランチきって
該当する部分のみPRする感じがよいのでしょうか?(作業を並走したことがないものでして…)

(部分的に作業がかさなっているので、コンフリクトします。分けない方がよかったですかね)

@Hiroshiba
Copy link
Member

PRお待ちしています!!

作法についてですが、正直特に無いと思います!
ちなみに今回は新しいPRが前のPRを必要としないパターンですよね。2つともPR出しちゃっていただくのが良いのかなと!

@nmori
Copy link
Contributor Author

nmori commented Feb 7, 2023

ありがとうございます。
では、個別のPRという形でだしてみます。

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants