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

refactor: AccentPhrase.vueを新規作成しAudioDetail.vueからアクセント句のコンポーネントを分離 Part.1 #1570

Merged
merged 34 commits into from
Sep 21, 2023

Conversation

thiramisu
Copy link
Contributor

@thiramisu thiramisu commented Sep 20, 2023

内容

#1569 から一部分割したもののその1です。

関連 Issue

その他

accentPhrase"s"を消す必要があるのと、element同士やelementと関数の関わりが強く一部ずつの移動が難しいのとで、機能を保ったままでの分割はこれ以上は難しいように感じています…。
コミット単位でもある程度意味のある塊に分割したつもりなので、github上でコミット順に流し見ていくと全体の流れが少しつかみやすくなるかもです。
それでもまだ辛そうでしたら、もう少し考えてみます。

@Hiroshiba
Copy link
Member

Hiroshiba commented Sep 20, 2023

分離ありがとうございます!!これだったら相当負担少なくレビューできると思います!!!

最後の方の 44ab7ee はPRタイトルから外れているかもです。
これだけは特別にこのコミットで差分を見てレビューすることも可能ではあるので、分離面倒であればコメント頂ければ 🙏
(あとからrevertしたいときにできない、くらい)

@Hiroshiba
Copy link
Member

@thiramisu もし切り分け作業してくださってたらすみません、今からレビューしたいと思います!

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.

プルリクエストありがとうございます!!!!
切り分けていただけて短時間でしっかりレビューできたと思います!!!

一つだけ消えてそうなのを見つけたのでコメントしました。


クラスの切り出しではないファクタリング範囲は別途レビューしたのでコメントです。

pronunciation周りのリファクタリング。OKそう。
b1da352

handleChangePronounce周りのリファクタリング。
OKだけど、これだけこっちのプルリクに入ってるのはちょっと文脈的に分かりづらいかも&色々一気にリファクタリングした方が見通しがよさそう。revertしてもいいかも。(そのままでもとりあえずOKです)
44ab7ee

src/components/AudioDetail.vue Show resolved Hide resolved
@thiramisu
Copy link
Contributor Author

thiramisu commented Sep 20, 2023

レビューありがとうございます!今やっとコメントの方確認しました、すいません。

handleChangePronounce
これに関しては、pronunciationByPhraseから次のアクセント句の情報が消えたのでそのまま持ってくるのは無理なんですよね。ただ今考え直したら変更は

-    newPronunciation += pronunciationByPhrase.value[phraseIndex + 1];
+    newPronunciation += "ア";

だけにして、とりあえず移動だけ優先するのはありかもです、まあこれでも若干中途半端さが残りますが…どうしましょうか?

@Hiroshiba
Copy link
Member

@thiramisu そこの変更は必須だと思います!
今思ったら,にリプレイスする機能が消えてるかも?(そもそもいるのかと言われると微妙ですが)

うーん!
まあでも変更作業お願いするのもあれなので、こっちでちょっと変更してマージさせていただこうと思います!

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

ずっとやりたかった切り出しなのでとても嬉しいです!!

handleChangePronounceの処理を元の形っぽく戻させていただいてからマージしたいと思います。

@Hiroshiba Hiroshiba merged commit 39701dd into VOICEVOX:main Sep 21, 2023
@thiramisu
Copy link
Contributor Author

変更ありがとうございます、確認しました。

,にリプレイスする機能が消えてる

以前のコードにgオプションがついてなかったのは微妙ポイントだったのですが、それはそれとして消すのもkanaRegex.testがfalseになるので問題ありそうでしたね…
以前のコードに戻していただいたので、次のPRでgオプションつけた形に書き直してみます。

@thiramisu thiramisu deleted the separate-accent-phrase-vue-1 branch September 21, 2023 13:05
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.

2 participants