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

feat: thiserror, tracingクレートの導入 #8

Merged
merged 11 commits into from
Jan 9, 2025
Merged

Conversation

HMasataka
Copy link
Contributor

@HMasataka HMasataka commented Oct 4, 2024

  • feat: accept player connection #4 の内容のthiserror, tracingクレートを導入する部分まで対応
  • tracingの初期化、関数のinstrument化を対応
  • Box<dyn Error> としていた箇所を全て共通のエラーとして処理出来るように発生したエラーをwrap

@HMasataka HMasataka marked this pull request as ready for review October 21, 2024 07:00
Comment on lines 9 to 28
#[error("initialize tracing subscriber error. {0}")]
InitializeTracingSubscriber(TryInitError),
#[error("create tcp listener error. err: {0}, port: {1}")]
CreateTCPListener(io::Error, u16),
#[error("accept new connection")]
AcceptNewConnection(io::Error),
#[error("accept new stream")]
AcceptNewStream(io::Error),
#[error("private key pem section not found")]
ReadPrivateKeyPEMSection,
#[error("unexpected rust ls error. {0}")]
BuildServer(rustls::Error),
#[error("failed to open file. {0}")]
OpenFile(io::Error),
#[error("failed to read file. {0}")]
ReadFile(io::Error),
#[error("failed to read buffer. {0}")]
ReadBuffer(io::Error),
#[error("failed to convert string. {0}")]
ConvertBuffer(FromUtf8Error),
Copy link
Contributor

Choose a reason for hiding this comment

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

ぜ、全体的にエラー文が雑……!
「ReadFile」や「OpenFile」とかのエラーはよくないです
代わりに、LoadPrivateKeyFileみたいな……意識する場所としては、「それを受けとったユーザーが何をすればいいのか」が分かるように……!
逆にそのようにエラーがまとまってれば、Openで失敗したかReadで失敗したかは割とどうでもいいわけです。
(きっと {0} のように参照される部分で表示されるからね!例えばFailed to load Private Key File: file not found みたいな)

また、エラー文の末尾にピリオドとかはつけません

いろいろ参考になるかも?
https://doc.rust-jp.rs/rust-by-example-ja/error/multiple_error_types/define_error_type.html

@AsPulse
Copy link
Contributor

AsPulse commented Oct 24, 2024

あと、プルリクエスト名がメインブランチへのコミットメッセージになっちゃうのでConventionalに従ってください

@HMasataka HMasataka force-pushed the feature/error_trace branch from 89b8850 to 2e510f5 Compare October 28, 2024 13:25
@HMasataka HMasataka changed the title thiserror, tracingクレートの導入 feat: thiserror, tracingクレートの導入 Jan 9, 2025
@HMasataka
Copy link
Contributor Author

@AsPulse
めっちゃ遅くなっちゃったけど更新したのでお手隙で見てくださいー!
79fc9dd

Copy link
Contributor

@AsPulse AsPulse left a comment

Choose a reason for hiding this comment

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

LGTM! + 1つ別のIssueに回します(レビュー参照)

clocking-server/src/main.rs Show resolved Hide resolved
@HMasataka HMasataka merged commit 887d13c into main Jan 9, 2025
4 checks passed
@HMasataka HMasataka deleted the feature/error_trace branch January 9, 2025 09:04
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