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

Add: Mecab_load_with_userdicを追加 #9

Merged

Conversation

sevenc-nanashi
Copy link
Member

@sevenc-nanashi sevenc-nanashi commented Jul 9, 2023

内容

Mecab_load_with_userdicを追加します。

関連 Issue

スクリーンショット・動画など

(なし)

その他

(なし)

@sevenc-nanashi sevenc-nanashi marked this pull request as draft July 9, 2023 02:56
@Hiroshiba
Copy link
Member

@sevenc-nanashi PRありです!!

もし知ってたらお聞きしたいのですが、この関数がなくてもpyopenjtalkの方はuserdicを読み込めてたのはなぜなんでしょう・・・?
(同じ方法を使えばこの関数がなくてもuserdicが読み込めるのかなと思った次第です。)

@sevenc-nanashi
Copy link
Member Author

https://github.com/VOICEVOX/pyopenjtalk/blob/827a3fc5c7dda7bbe832c0c69da98e39cc8cb2c3/pyopenjtalk/openjtalk.pyx#L28-L55

ユーザー辞書指定がなければMecab_load、指定されていたらMecab_load_with_userdic(のようなもの)をCPythonで実装、のようになっています。

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

mecabのコードを追っていました。結構不思議な作りですね!
調整ありがとうございます!!!

src/mecab/src/mecab.cpp Outdated Show resolved Hide resolved
src/mecab/src/mecab.cpp Outdated Show resolved Hide resolved
src/mecab/src/mecab.cpp Outdated Show resolved Hide resolved
@Hiroshiba
Copy link
Member

あ、pyopenjtalk側もこれ使うようにするのが良いかもですかね。
マージ後にisuse立てときますか!(忘れそう)

@sevenc-nanashi
Copy link
Member Author

レビューを反映しました。

あ、pyopenjtalk側もこれ使うようにするのが良いかもですかね。

確かに。良さそう。

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

辞書に詳しい @takana-v さんももしよかったらちょっと眺めていただけると心強いです・・・!!

@y-chan VOICEVOXのopenjtalkに珍しく機能追加が入るので共有です 🙏

Copy link
Member

@takana-v takana-v left a comment

Choose a reason for hiding this comment

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

(C++に詳しくないのでちょっと自信無いですが)LGTM!

@sevenc-nanashi
Copy link
Member Author

Approve集まったのでマージします。

@sevenc-nanashi sevenc-nanashi merged commit cb555f5 into VOICEVOX:1.11 Jul 10, 2023
@sevenc-nanashi sevenc-nanashi deleted the add/mecab_load_with_userdic branch July 10, 2023 11:10
return Mecab_load_with_userdic(m, dicdir, NULL);
}

void Mecab_print_load_error(const char *dicdir, const char *userdic) {
Copy link
Member

@qryxip qryxip Jul 11, 2023

Choose a reason for hiding this comment

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

今言うのは微妙ですが、ここはstatic voidにしてMecab_というprefixを外した方がよかった気がします。

Copy link
Member

Choose a reason for hiding this comment

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

たしかに。private関数だからMecab_いらなかった。

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.

4 participants