Skip to content

パッケージを分割した#34

Open
shokkunrf wants to merge 7 commits into
developfrom
refactor-package-all
Open

パッケージを分割した#34
shokkunrf wants to merge 7 commits into
developfrom
refactor-package-all

Conversation

@shokkunrf
Copy link
Copy Markdown
Member

ECS対応の前段階として、パッケージを分けた
エラーハンドリングのベストプラクティスがわからなかったため、実装の改善案などあれば取り入れていきたい
"このあたり受け入れがたい実装をしているけどECS対応のときに変更できればいいや"みたいなコメントでもOK

@shokkunrf shokkunrf force-pushed the refactor-package-all branch from 5999edb to f4d6086 Compare January 29, 2022 13:21
Copy link
Copy Markdown
Member

@usagiga usagiga left a comment

Choose a reason for hiding this comment

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

詳細な実装に依存しない

以下の関数をまとめた、「インスタンスを操作する interface 」を定義して、それを使う方がよいと思います。今後テストを追加したときにモックを追加しやすくしたり、ECS以外の実行環境が増えたときに変更を局所的にしたりするためです。

  • GetIPAddress()
  • StartInstance()
  • StopInstance()

ここは今後 ECS 化対応する時に対応するでもいいです。

エラーハンドリングについて

discord パッケージで discord 向けのエラーメッセージを出しているのは自然なのですが、
aws パッケージのエラーメッセージは discord パッケージに流出してしまっています。

  • var ErrHogeFuga error を使いまわす方がよい
    • ErrHogeFuga 自体を等値比較したり、 errors.Is() したりできるためです
    • エラーメッセージもエラー定義時に書けるので、責務が流出しません
    • database/sql など、色々なところで使われています
  • エラーコードは不要かもしれない
    • 上の修正をするとエラーコードを要求する箇所がなくなります
    • 本来のエラーコードは、バックエンドのエラーをマスクしつつ、フロントエンドでエラーコードを表示しておいて、対応方法マニュアルを参照してもらったり、CSや運用部隊が原因調査するのに使ったり、という用途なような気がします
  • 各関数が返したエラーを握りつぶさないようにしたい
    • Go 1.13 errorspkg/errors を使えば、もとのエラーを包んだエラーを作ることができます

Go 1.13 errors を使うとするならこんな形になるでしょうか。
https://go.dev/play/p/WhIcyWn0DBT

もしスタックトレースを出したい場合は pkg/errors を使うといいと思います。

Go のエラーハンドリング

知る限り、メジャーどころは以下の3つのライブラリです。
これらを使っていれば自然にエラーハンドリングが出来るはずです。

  • Go 1.13 errors
    • fmt.Errorf("hoge: %w", err) でラップしたり、errors.Is() で比較したりできます
    • スタックトレースが 吐けません
    • 標準ライブラリなのが good
  • pkg/errors
    • スタックトレースが吐けます
      • 表示すると os.Exit() が叩かれているような気がする(謎)
    • Go 2 のエラーの proposal とは違うインタフェース「も」使えるようになってます
      • Go 2 っぽいエラーのインタフェース「も」あります。お好みで使い分けましょう
    • メンテナンスが継続中
  • xerrors
    • Go 2 のエラーの proposal を試験的に実装したライブラリです
    • スタックトレースが吐けます
    • 標準ライブラリ( fmt errors とか)との親和性が高いです
    • Go 1.13 に一部の機能が取り込まれたことでメンテナンスが打ち切られています

Comment thread src/aws/error.go Outdated
package aws

const (
ERR_FAILED_START_INSTANCE = iota
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

iota + 11 << iota (複数のフラグを同時に立てたいとき)を使う方がよいです。

iota は 0 から始まるのですが、これは int のデフォルト値と同じなので、意図しない挙動が発生することがあります。
今回のコードだと、以下のような状況で StatusError.IsEmpty が誤って true になります。

err := StatusError{
	Code: ERR_FAILED_START_INSTANCE,
	Err:  "",
}
err.IsEmpty() // true

Comment thread src/discord/bot-receive.go Outdated
errLogMsg = "停止待ちに失敗した :"
errDiscordMsg = "インスタンスの停止状態不明 再度のコマンド入力を要求"
}
log.Println(errLogMsg, err)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

ここの err はいかなるときでも nil なので、表示しなくてもいいです。

@usagiga
Copy link
Copy Markdown
Member

usagiga commented Jan 29, 2022

また、 CI が落ちているので、 CI のワーキングディレクトリを src に変えてやってください。:bow:
https://docs.github.com/en/actions/using-workflows/workflow-syntax-for-github-actions#example-set-the-default-shell-and-working-directory

@shokkunrf shokkunrf force-pushed the refactor-package-all branch from 86e5d18 to a85196b Compare January 31, 2022 10:00
@shokkunrf
Copy link
Copy Markdown
Member Author

レビューありがとうございます
コメントやデモコードなど本当に助かりました:pray:

エラーハンドリングについて
hanselの規模的にスタックトレースはなくてもいいかなとの判断で errors を利用しました
必要になってから pkg/errors に移行するのでも遅くはないのかなと思います (その頃には標準ライブラリに輸入されてればいいな)

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