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

VoicevoxResultCodeをC APIに移動 #580

Merged
merged 1 commit into from
Aug 22, 2023

Conversation

qryxip
Copy link
Member

@qryxip qryxip commented Aug 22, 2023

内容

VoicevoxResultCodeをC APIに移動します。

#275 (comment)

関連 Issue

Closes #275.

その他

future workとして以下のことを考えています。このPRがマージされ次第取り掛かろうかと。

  • voicevox_core::Errorを再編する
    • 各ドメインごとに適切に分割し、anyhow::Errorをできるだけ無くす
    • ResultCodeと名前が微妙にずれているものがまだあるのでそれも統一
    • codeも、v0.14と差分が小さくなるように再編 (mutabilityとasyncnessを仕上げる #553 (comment))
  • エラーメッセージの見直し
    (表記ゆれ、"speaker_id"、…)
  • C APIでの表示をtracing::error!で出す
  • Display実装にはsourceの内容を含めない
    (これは慣習としてどこかで明文化されていたはずですが、それがどこなのかを忘れました)
  • Python APIとJava APIでsourceの内容を表現
  • その他

@qryxip qryxip requested a review from Hiroshiba August 22, 2023 17:07
Copy link
Member Author

@qryxip qryxip left a comment

Choose a reason for hiding this comment

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

補足:

Comment on lines -20 to -24
macro_rules! cstr {
($s:literal $(,)?) => {
CStr::from_bytes_with_nul(concat!($s, '\0').as_ref()).unwrap()
};
}
Copy link
Member Author

Choose a reason for hiding this comment

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

cstrを導入したことによりついでに削除

Comment on lines +52 to +75
result_messages.0 = "エラーが発生しませんでした"
result_messages.1 = "OpenJTalkの辞書が読み込まれていません"
result_messages.3 = "サポートされているデバイス情報取得中にエラーが発生しました"
result_messages.4 = "GPU機能をサポートすることができません"
result_messages.6 = "無効なspeaker_idです"
result_messages.7 = "無効なmodel_idです"
result_messages.8 = "推論に失敗しました"
result_messages.11 = "入力テキストからのフルコンテキストラベル抽出に失敗しました"
result_messages.12 = "入力テキストが無効なUTF-8データでした"
result_messages.13 = "入力テキストをAquesTalk風記法としてパースすることに失敗しました"
result_messages.14 = "無効なaudio_queryです"
result_messages.15 = "無効なaccent_phraseです"
result_messages.16 = "ZIPファイルのオープンに失敗しました"
result_messages.17 = "ZIP内のファイルを読むことができませんでした"
result_messages.18 = "同じIDのモデルを読むことはできません"
result_messages.26 = "同じIDのスタイルを読むことはできません"
result_messages.27 = "モデルデータを読むことができませんでした"
result_messages.19 = "Modelが読み込まれていません"
result_messages.20 = "ユーザー辞書を読み込めませんでした"
result_messages.21 = "ユーザー辞書を書き込めませんでした"
result_messages.22 = "ユーザー辞書に単語が見つかりませんでした"
result_messages.23 = "OpenJTalkのユーザー辞書の設定に失敗しました"
result_messages.24 = "ユーザー辞書の単語のバリデーションに失敗しました"
result_messages.25 = "UUIDの変換に失敗しました"
Copy link
Member Author

Choose a reason for hiding this comment

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

voicevox_core_c_apiをrlibとして読むことも普通にできはするが、同じvoicevox_coreという名前でrlibとcdylibが同居したときにどうなるかわからないのでそれはやめることにした。

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

設計方針に関するコメントです!

  • 方針は全て賛成です。
  • エラーコードはジャンルごとに分けて桁数を大きくしてもいいかも。辞書周りは1000番台、とか。
    • でも大きな差分になってしまうのでv1アプデの時のが良いかも。
  • utility関数のResultはanyhow::Errorが良いかも
    • それが何のエラーになるか関数内で分からない(誰かにラップされ直す)
    • 表にも出ないから無駄に名付けコストがかかってしまうので
    • ちゃんとしたエラー設計を知らないので雰囲気で言ってます。。
    • anyhow::Errorしか知らないからこう言ってるだけです、他に良いResultがあればそれでも)
    • 全然強い意見じゃないです。anyhow::Errorを使うプルリクエストが来ても通す、自分でエラーを書いてあるリクエストも通す、みたいな気持ち。

@Hiroshiba
Copy link
Member

問題ないと思うのでマージします!

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.

error_result_to_message関数で返ってくる文字列がnull終端文字列になってしまっている
2 participants