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: VvppManagerをリファクタリング #2551

Merged

Conversation

Hiroshiba
Copy link
Member

内容

VvppManagerをリファクタリングしてみました。
markしてhandleする機構上、これ以上は割と難しそうかも。

ついでにバグを2つ直してます。

  • パーミッションを変更してから移動するようにした
    • 以前は移動してからパーミッション変更してた
    • markで移動を予約した後はパーミッションを変更してないので、おそらくバグってた
  • 移動の際に削除するエンジンディレクトリを、toディレクトリではなく毎回取得するようにした
    • 前はtoディレクトリのものを消していた
    • エンジンIDそのままに名が変わったときなどはディレクトリが変わるので、toディレクトリを消すだけではダメなときもある

いずれも関数化して処理を整理していくと割と問題が見えてきたので、関数に切り分けていくのは大事だなと改めて思いました。

関連 Issue

の続き

その他

@Hiroshiba Hiroshiba requested a review from a team as a code owner February 20, 2025 04:39
@Hiroshiba Hiroshiba requested review from sevenc-nanashi and Copilot and removed request for sevenc-nanashi February 20, 2025 04:39
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (3)

src/backend/electron/manager/vvppManager.ts:71

  • [nitpick] Consider destructuring the parameters directly in the function signature for clarity.
markWillMove(params: { from: string; to: string; engineId: EngineId }) {

src/backend/electron/manager/vvppManager.ts:213

  • Include the from and to paths in the error message for more context.
log.error("Failed to rename engine directory: ", e);

src/backend/electron/manager/vvppManager.ts:255

  • Include the error message in the log to provide more context about why the retry is happening.
log.warn(`Retrying... (${i + 1}/${maxRetries}):`);

@Hiroshiba Hiroshiba requested a review from Copilot February 20, 2025 04:42

Choose a reason for hiding this comment

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

Copilot reviewed 1 out of 1 changed files in this pull request and generated no comments.

Comments suppressed due to low confidence (3)

src/backend/electron/manager/vvppManager.ts:97

  • [nitpick] The error message 'Multiple installed engine directories found.' is unclear. Consider providing more details or instructions for the user.
throw new Error("Multiple installed engine directories found.");

src/backend/electron/manager/vvppManager.ts:249

  • [nitpick] The function name 'retry' is ambiguous. Consider renaming it to 'retryOperation' or something more descriptive.
async function retry(fn: () => Promise<void>) {

src/backend/electron/manager/vvppManager.ts:249

  • Ensure that the new behavior introduced by the 'retry' function is covered by tests.
async function retry(fn: () => Promise<void>) {
@voicevox-preview-pages
Copy link

voicevox-preview-pages bot commented Feb 20, 2025

🚀 プレビュー用ページを作成しました 🚀

更新時点でのコミットハッシュ:30f3340

Copy link
Member

@sevenc-nanashi sevenc-nanashi left a comment

Choose a reason for hiding this comment

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

細かい所をいくつか。

Copy link
Member

@sevenc-nanashi sevenc-nanashi left a comment

Choose a reason for hiding this comment

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

再Approve。

@Hiroshiba
Copy link
Member Author

あっ一度approveもらってたの気づいてませんでした!! 🙇

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

@Hiroshiba Hiroshiba enabled auto-merge February 20, 2025 07:53
@Hiroshiba Hiroshiba added this pull request to the merge queue Feb 20, 2025
Merged via the queue into VOICEVOX:main with commit 55ad4ba Feb 20, 2025
11 checks passed
@Hiroshiba Hiroshiba deleted the VvppManagerのリファクタリング branch February 20, 2025 08:19
sevenc-nanashi pushed a commit to sevenc-nanashi/voicevox that referenced this pull request Feb 21, 2025
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