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

Coloring #21

Merged
merged 8 commits into from
Aug 15, 2022
Merged

Coloring #21

merged 8 commits into from
Aug 15, 2022

Conversation

ccycle
Copy link
Contributor

@ccycle ccycle commented Aug 10, 2022

LogLevelにあわせて標準出力の一部に色を付ける機能を実装した

@ccycle ccycle requested a review from fumieval August 10, 2022 09:08
Copy link
Contributor

@fumieval fumieval left a comment

Choose a reason for hiding this comment

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

モジュールを複製して一部分だけ変えるのは、保守性の観点で良くないです。pushLogStrLn を関数の引数として受け取るなどして重複コードが増えないようにしましょう

import System.Console.ANSI

-- https://hackage.haskell.org/package/ansi-terminal-0.11.3/docs/System-Console-ANSI-Types.html#t:SGR
data SGRLogInfo = NotColored | SGRLogInfo [SGR]
Copy link
Contributor

Choose a reason for hiding this comment

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

NotColoredが使われていないようですね。仮に使ったとしても SGRLogInfo []と役割が変わらないため、SGRLogInfoを定義する必要性は低そうです


stdoutANSITransport :: LoggerSet -> LogLevel -> Transport
stdoutANSITransport loggerSet transportThreshold =
let name = "stdout"
Copy link
Contributor

Choose a reason for hiding this comment

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

この名前はTransportの実装ごとに異なることが期待されています

)
let value = A.pairs $ series <> foldMap (uncurry (.=)) (KM.toList extra)
let json = A.encodingToLazyByteString value
-- pushLogStrLn loggerSet (toLogStr json)
Copy link
Contributor

Choose a reason for hiding this comment

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

特別な理由がない限り、コメントアウトしたコードは残さないようにしましょう

Copy link
Contributor

@fumieval fumieval left a comment

Choose a reason for hiding this comment

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

masterに #20 がマージされたので、このブランチをリベースし、テスト結果のスクリーンショットの貼り付けをお願いしたいです

@@ -18,6 +18,8 @@ library
Herp.Logger.SentryTransport
Herp.Logger.StdoutTransport
Herp.Logger.Transport
Herp.Logger.ANSI.Coloring
Herp.Logger.StdoutANSITransport
Copy link
Contributor

Choose a reason for hiding this comment

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

このモジュールはもう存在しないですね(コミット忘れ?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

すみません、その部分の修正を見落としてました。今pushしたcommitの分で修正しました。

@ccycle
Copy link
Contributor Author

ccycle commented Aug 15, 2022

了解です。その際色付きの分のテストケースを stdout.hs に追加してもよろしいでしょうか?

@fumieval
Copy link
Contributor

了解です。その際色付きの分のテストケースを stdout.hs に追加してもよろしいでしょうか?

お願いします!

@ccycle
Copy link
Contributor Author

ccycle commented Aug 15, 2022

branchをrebaseし、テストを追加してみました。以下テスト結果のスクリーンショットです:

色なし:
Screen Shot 2022-08-15 at 11 32 10

色付き:
Screen Shot 2022-08-15 at 11 32 17

<> "date" .= T.decodeUtf8 (SB.fromShort date)
<> "message" .= message
)
value = A.pairs $ series <> foldMap (uncurry (.=)) (KM.toList extra)
Copy link
Contributor

@fumieval fumieval Aug 15, 2022

Choose a reason for hiding this comment

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

valueを束縛する必要がなくなっていますね

tests/stdout.hs Outdated
loggerLevelTest loggerSet [stdoutTransport loggerSet Debug] Debug
loggerLevelTest loggerSet [stdoutTransport loggerSet Debug] Informational
loggerLevelTest loggerSet [stdoutANSITransport loggerSet Debug] Debug
loggerLevelTest loggerSet [stdoutANSITransport loggerSet Debug] Informational
Copy link
Contributor

Choose a reason for hiding this comment

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

スレッショルドのロジックは上でテストされているので、ここで改めてテストする必要はないと思います

@ccycle
Copy link
Contributor Author

ccycle commented Aug 15, 2022

コメントありがとうございます、反映させてみました。これだとどうでしょうか?

Copy link
Contributor

@fumieval fumieval left a comment

Choose a reason for hiding this comment

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

ありがとうございます!

@ccycle ccycle merged commit 385c4b5 into master Aug 15, 2022
@fumieval
Copy link
Contributor

あ、マージする前にコミットをまとめて欲しかったですが、まあ今回はいいでしょう

@ccycle
Copy link
Contributor Author

ccycle commented Aug 15, 2022

しまった、忘れてました…次からは気をつけます。

@ccycle ccycle deleted the coloring branch August 15, 2022 03:03
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