-
Notifications
You must be signed in to change notification settings - Fork 312
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
feat: 疑問文自動調整設定機能追加 #610
feat: 疑問文自動調整設定機能追加 #610
Conversation
169939f
to
f9d57f3
Compare
openapiのコード生成について、以前生成したときから定義が変わっていないところについても生成されたコードが変わってるようなのでちょっと気になります |
openapiのコード生成を行った refs VOICEVOX#608 VOICEVOX#610
f28c3ab
to
a1c1160
Compare
a1c1160
to
257f5c4
Compare
257f5c4
to
084da03
Compare
もしかしたらいらない配慮かもしれないが、 ExperimentalSettingオブジェクト型を作成し、その中にenableInterrogativeを持たせるようにした。 enableInterrogativeは現状実験的な機能だが、将来的に正式な機能として提供された場合デフォルト設定値をtrueにする可能性が高そうである。 これをSettingStoreStateに対してenableInterrogativeを直接持たせてそのままの状態でenableInterrogative機能が正式リリースされた場合、既存ユーザーはenableInterrogativeがfalseの状態のまま追加居続けるのではないか? との疑念があったのでExperimentalSettingを作成してネストさせることにしてみた。もしかしたら違うかも。 こうすることによってenableInterrogative機能を正式リリースする際にstate.experimentalSetting.enableInterrogativeからstate.enableInterrogativeに構造を変えることによってデフォルト値trueを適用した状態で既存ユーザーにも機能を提供できるようになるはずである。
084da03
to
798878b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
設定フィールド一つ増やすのにここまで変更が必要なのかやや疑問なのですが、こういうものなのでしょうか。
ExperimentalSettingという型を作りネストさせてenableInterrogativeフィールドをもたせましたが余計だったかもしれません。
ただ将来的に疑問文が正式対応されたときにデフォルト値をtrueにしたかった場合、アップデート後既存ユーザーに疑問文が有効な状態で提供したほうが良いかなとおもってこうしました。
違ったら直します。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
レビュワーは @Hiroshiba と @y-chan でいってみますか!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
コメントの有無など、コードの意図を汲み取って書いて頂けていてすごく読みやすかったです!!
設定フィールド一つ増やすのにここまで変更が必要なのかやや疑問なのですが、こういうものなのでしょうか。
typescriptを極めるとおそらく半分くらい省けそうなのですが、間に合わずこうなっている感じです。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!!
@y-chan さんもお時間あるときに見て頂けると!
src/background.ts
Outdated
@@ -274,9 +274,11 @@ const store = new Store<{ | |||
properties: { | |||
enableInterrogative: { | |||
type: "boolean", | |||
default: false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ちょっと良く覚えてないのですが、たしかこっちのdefault値も必要だった記憶がぼんやりあります。
下のdefaultはdictがなかったときに追加されて、こっちのdefaultはdictの要素がなかったときに使われる感じです。
ということで次のコメントのsuggestを適用して頂けると!
必要っぽい?ので Co-authored-by: Hiroshiba <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
特に大きく問題はないと思いました!
次のバージョンで疑問文が使えるようになるのが楽しみです....!
内容
Engineに疑問文自動調整機能が追加されたので、front側でその有効無効を調整できるようにする
関連 Issue
refs #608
スクリーンショット・動画など
その他