Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Rust APIが公開するエラーの種類を
ErrorKind
として表現する #589Rust APIが公開するエラーの種類を
ErrorKind
として表現する #589Changes from 5 commits
e5ac602
de3c38b
e8045e4
d4b8b0b
d6489cc
58a3558
1020465
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
各エラー型の可視性の降格により、シグネチャにそれらのエラー型が表われる関数がコンパイルエラーになるので同様に可視性を調整。
これによりコード中に特に理由無く
pub
とpub(crate)
が混在することになりますが、Rust API内のアイテムの可視性については元々深く考えられていないということで今のところはよいかなと思いました。There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
混じってしまうのはまあありなのかなと思いました。
ただ混じってしまうぐらいなんだったら、今は全部統一的に
pub
にしておいて、pub(crate)
にする機会を別途作成してそこで一気に変更とかもありかもとか思いました。まあでもせっかく書いちゃったんだし、こちらのプルリクエストの形で一旦マージするのもありかもです。
あまり強い意見ではないです。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#594 を作りました。
(初心者歓迎タスクにしました)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
多分できることがError==ErrorReprだと思うのですが、であれば最初から内部で使う構造体もErrorで良いのではとちょっと思いました。
メリットがあれば知りたいです。
あるいは一般的にクレート内のエラー構造体と外に出すエラー構造体はレイヤーが異なるため型を分けるべきである、みたいな思想があるのであれば一旦従うのもありかなと思いました。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
こちらのコメントで納得したのでResolveです!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
UnloadedModel
と統合される形になると思ってます。There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ErrorKind
の定義および変換をしてくれるライブラリがあった。kinded
作者はnutypeやwhatlangを作った人。強制的に
Display
/FromStr
が付く点には異議を唱えたいところではあるが、それ以外は筋がよいと感じるし、実装のコードも簡潔。が、initial commitが28日前、最終更新日が21日前で、総ダウンロード数658…
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
もうちょっと待ってみて、ライブラリ側やこちら側も安定して来てそうだったら導入を検討しても良さそうなのかなと思いました!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ここのdocは現状の
ResultCode
からそのまま持って来ました。