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

change: manifest_versionvvm_format_version #794

Conversation

qryxip
Copy link
Member

@qryxip qryxip commented May 23, 2024

内容

#581 (comment)の1.です。manifest_version (SemVer)をvvm_format_version (整数値を文字列化したもの)にします。

実装は#581 (comment)で書いたやつの冗長な方を持って来ました。
(エラーメッセージにこだわらないのならenumのやつでもいいかもしれません)

関連 Issue

ref #581

その他

@qryxip qryxip requested a review from Hiroshiba May 23, 2024 15:23
@sevenc-nanashi
Copy link
Member

個人的にはenumでもいいかな~って思います。
「vvmの読み込みに失敗しました。Coreが対応していないバージョンを読み込もうとしたか、vvmファイルが破損しています。」みたいなメッセージを呼び出し側で出せばそれっぽくなりそう?

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

実装ありがとうございます!! 🙇

vvm_format_versionの認識ですが、「VVMのmanifest.jsonやバイナリフォーマットが変わり、今までのコアが正しくVVMを扱えなくなったらインクリメントする」という認識です!
このあたり

1箇所コメントしてますが、変えても変えなくても問題なさそう!
変えても変えていなくてもマージしちゃっていただければ!

model/sample.vvm/manifest.json Outdated Show resolved Hide resolved
crates/voicevox_core/src/manifest.rs Outdated Show resolved Hide resolved
@Hiroshiba
Copy link
Member

@sevenc-nanashi 個人的には、そのメッセージが表示されたらVVMの破損を疑う気がします。
コアを使ってくれる開発者の人の時間を本質的なところに当ててほしいので、よく遭遇しそうなエラーは丁寧に作ってあげた方が良いかなと・・・!

@qryxip
Copy link
Member Author

qryxip commented May 24, 2024

バージョンはなんとなく文字列にしたのですが、Cargo.lock見たら整数値だったし整数値の方がいいのかも?
(追記) エディタのRuntimeInfoのfileFormatVersion1ですよね

@qryxip qryxip requested a review from Hiroshiba May 24, 2024 16:13
@Hiroshiba
Copy link
Member

Hiroshiba commented May 24, 2024

バージョンはなんとなく文字列にしたのですが、Cargo.lock見たら整数値だったし整数値の方がいいのかも?
(追記) エディタのRuntimeInfoのfileFormatVersionも1ですよね

確かにです!!
semverがあり得る場合は文字列にした方が良いかもです!

追記:そこさえ変えればマージしていただいても大丈夫そう!

@qryxip
Copy link
Member Author

qryxip commented May 24, 2024

整数にしました。
c38b9a4 (#794)

@qryxip qryxip removed the request for review from Hiroshiba May 24, 2024 17:40
@qryxip qryxip merged commit be8d0c0 into VOICEVOX:main May 24, 2024
31 checks passed
@qryxip qryxip deleted the change/change-manifest-version-to-vvm-format-version branch May 24, 2024 17:40
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