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

追加:AudioQuery.pauseLengthのSkipJsonSchemaをなくす #1430

Merged

Conversation

Hiroshiba
Copy link
Member

内容

AudioQueryのpauseLengthは珍しくNone(null)を受け入れ可能なのですが、SkipJsonSchemaがついていてopenapi.jsonの型的に受付不可に見えてしまっています。
外して受付可能だということがわかるようにします。

ついでに、古いAudioQueryを使って音声合成APIを叩くテストも実装しました。

関連 Issue

その他

@Hiroshiba Hiroshiba requested a review from a team as a code owner June 24, 2024 18:44
Copy link
Member Author

@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.

@sabonerune @tarepan もしよかったら見ていただけると助かります!!

@tarepan
Copy link
Contributor

tarepan commented Jun 25, 2024

AudioQueryのpauseLengthは珍しくNone(null)を受け入れ可能

👍️
この理解自体は正しいです。

ただし、これをスキーマへ反映させる場合は注意が必要そうです。なぜなら undefined と null が同じ意味か違う意味かユーザーにはわからないからです。
API 設計によっては「pauseLength undefined はデフォルト動作」「null は積極的な "無" なので pauseLength=null はpause_length = 0」と使い分ける場合もあります。VOICEVOX だと早口モードみたいな使い方で pau=0 で動作する可能性もあるため、尚更どういう意味か曖昧になりそうです。

ゆえに以下の点について明確化する必要があります:

  • ENGINE の動作は undefined と null でどう定義されているか、それは意図してそう定義されているか
  • undefined と null の動作の相違はユーザーへ案内されているか

@Hiroshiba
Copy link
Member Author

たしかにnullのときの意味は案内が必要そうです!

undefinedのとき(指定しなかったとき)の動作は、「デフォルト値は◯◯」とすると良いかも。

AudioQueryのパラメータはどういうときにどうなる値なのか説明が全くないのですが、pauseLengthのnullは特にややこしいので説明を追加したいと思います。

@Hiroshiba
Copy link
Member Author

ついでにnullのときの処理とデフォルト値を追記しました。

「nullのときは無視される」は、無視された結果どうなるかがわからないのですが、まあそれは他のパラメータもそもそもどういう結果になるかわからないので、全パラメータ詳しくするとなったときで良いかなと。

Copy link
Contributor

@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👍️

新機能の意図が明確に型・スキーマ・ドキュメントで示されています。great!

@Hiroshiba
Copy link
Member Author

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

@Hiroshiba Hiroshiba merged commit ca83fa5 into VOICEVOX:master Jun 26, 2024
4 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.

None yet

2 participants