feat: [project-sequencer-statemachine] 初期ステートの設定周りを変更 #2518
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
sigprogramming
requested review from
Hiroshiba
and removed request for
a team
16 tasks
|
🚀 プレビュー用ページを作成しました 🚀 更新時点でのコミットハッシュ： |
Hiroshiba
reviewed
Feb 3, 2025
Hiroshiba
approved these changes
Feb 3, 2025
There was a problem hiding this comment.
LGTM！！
ちょっと一箇所変えさせていただこうと思います！
PR内容にコメントの空行を削除とあったのですが、 95e6085 で元に戻ってそうでした！
どっちが意図した変更なのかわからないですが、とりあえず今はプルリクエストの前と実装が変わってない状態なので一旦良さそう。
意図していなかった場合は後で変えていただければ･･･！
ちなみに空行を追加するか消すかのコメントスタイルはどっちでも良さそうですが、コード全体で統一されてることは大事だと思います！
個人的には･･･改行なくても全然見やすそう＆一覧性が上がりそう！
まあそもそも@paramがなくてもわかるような引数名にすればいい気がします！
どうしてもわからないのだけコメントを書く感じとかでも。
例えばtransitionToの@param id 遷移先のステートのID。とかは、stateIdという引数名にすればかなり自明かなと。
（これの場合は型でStateIdだと分かってるので、idのままでも良さそう。）
| @@ -91,6 +91,15 @@ type StateFactories<T extends StateDefinition[], Input, Context> = { | |||
| ) => State<T, Input, Context> & { readonly id: U }; | |||
| }; | |||
|
|
|||
| /** | |||
| * 初期ステートとして設定可能なステートのIDを表す型。 | |||
There was a problem hiding this comment.
「設定可能なステートのID」と書いていて1つの ID ではないことがわかるので、「表す型」は自明かも。
Suggested change
| * 初期ステートとして設定可能なステートのIDを表す型。 | |
| * 初期ステートとして設定可能なステートのID。 |
だけど他の定義にはそう書いてあるので、ここは書いた方が良さそう！
変えるなら全部変えた方が良さそう。
Hiroshiba
reviewed
Feb 3, 2025
Hiroshiba
changed the title
Hiroshiba
merged commit 4205802
into
VOICEVOX:project-sequencer-statemachine
10 checks passed
sigprogramming
deleted the
change_initial_value_to_be_given_from_outside
branch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
内容
初期ステートをIDで指定する形に変更して、useSequencerStateMachineする際に指定できるようにします。
（デフォルト値を無くす）
また、以下も行います。
関連 Issue
その他