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: extractVvpp周りのTODOを解決 #2499

Merged
merged 9 commits into from
Jan 22, 2025

Conversation

Hiroshiba
Copy link
Member

@Hiroshiba Hiroshiba commented Jan 21, 2025

内容

で後回しにしたTODO系を片付けました。

その他

@Hiroshiba Hiroshiba requested a review from a team as a code owner January 21, 2025 10:48
@voicevox-preview-pages
Copy link

voicevox-preview-pages bot commented Jan 21, 2025

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

更新時点でのコミットハッシュ:9ee5f33

tests/unit/backend/electron/vvppFile.node.spec.ts Outdated Show resolved Hide resolved
Comment on lines +224 to +229
// https://www.garykessler.net/library/file_sigs.html#:~:text=7-zip%20compressed%20file
const SEVEN_ZIP_MAGIC_NUMBER = Buffer.from([
0x37, 0x7a, 0xbc, 0xaf, 0x27, 0x1c,
]);

const ZIP_MAGIC_NUMBER = Buffer.from([0x50, 0x4b, 0x03, 0x04]);
Copy link
Member Author

Choose a reason for hiding this comment

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

スコープは短いほど良いので関数内に移動・・・
というよりこれは単純に場所が近いほうがわかりやすいからというのが大きそう。

{
vvppLikeFilePath: vvppPath,
vvppEngineDir: this.vvppEngineDir,
tmpDir: app.getPath("temp"),
Copy link
Member Author

Choose a reason for hiding this comment

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

vvpppを結合したものができるファイル、やっぱりelectronのtempディレクトリに置くようにしました。
これが一番明確だなぁと。

@Hiroshiba Hiroshiba changed the title refactor!: extractVvpp周りのTODOを解決 refactor: extractVvpp周りのTODOを解決 Jan 21, 2025
Copy link
Member Author

@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.

@sevenc-nanashi よかったらまたレビューを・・・!!

ちなみにメンションしなくてもReviewersへの追加だけで良かったりしますか・・・? 👀

@sevenc-nanashi
Copy link
Member

Reviewer追加でもGitHub Mobileの通知とかは来ますね。

@Hiroshiba
Copy link
Member Author

あっなるほどです!!
これからはassignだけにしてみます。
とりあえずvvpp周りは名無し。さんが書いてくださったとこでロジックの共通認識があると思うので、頼らせていただければと・・・!!! 🙇

Copy link
Member Author

@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.

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.

良さそう。

@Hiroshiba
Copy link
Member Author

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

@Hiroshiba Hiroshiba enabled auto-merge January 22, 2025 01:59
@Hiroshiba Hiroshiba added this pull request to the merge queue Jan 22, 2025
Merged via the queue into VOICEVOX:main with commit 3c69896 Jan 22, 2025
11 checks passed
@Hiroshiba Hiroshiba deleted the hiho-20250121-194844 branch January 22, 2025 03:38
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