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

audio.tsのリファクタリング #1832

Merged
merged 5 commits into from
Feb 13, 2024
Merged

Conversation

weweweok
Copy link
Contributor

@weweweok weweweok commented Feb 8, 2024

内容

題の通りです。

その他

@weweweok weweweok requested a review from a team as a code owner February 8, 2024 07:51
@weweweok weweweok requested review from y-chan and removed request for a team February 8, 2024 07:51
Copy link
Member

@y-chan y-chan left a comment

Choose a reason for hiding this comment

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

LGTM!

ロジックが大きく変わるところはなさそうで、コード的に読みやすくなったので良いと思います!

src/store/audio.ts Outdated Show resolved Hide resolved
@weweweok
Copy link
Contributor Author

レビューを反映しました。

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です!!
1箇所だけ、改良したことにより意図不明になっているところがありそうだったのでコメントしてみました!

src/store/audio.ts Outdated Show resolved Hide resolved
Co-authored-by: Hiroshiba <hihokaruta@gmail.com>
@Hiroshiba
Copy link
Member

変更ありがとうございます!!
ちなみに動作チェックしてないのですが、大丈夫そうでしたか🙇
(たぶんここはテストないので…)

@weweweok
Copy link
Contributor Author

weweweok commented Feb 12, 2024

Math.maxあり・なしで、イントネーションクリック時(set関数が起動するタイミング)の挙動を比べてみました。
結果を見比べた限り、正常に動いているのではないかと...!

実験箇所が少々心配ですが、いかがでしょうか?

Math.maxあり

Screencast.from.02-13-2024.08.28.26.AM.webm

Math.Maxなし

Screencast.from.02-13-2024.08.31.11.AM.webm

console.log設置箇所

image

@Hiroshiba
Copy link
Member

ありがとうございます、大丈夫そうかなと思いました!!!

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!!!

リファクタリングは大事なのですが、なかなか手がつけられずにいるのでものすごい助かります!!
これからももしよかったら是非!!

@Hiroshiba Hiroshiba merged commit e936796 into VOICEVOX:main Feb 13, 2024
9 checks passed
@weweweok weweweok deleted the refactor branch February 13, 2024 02:14
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.

3 participants