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: add CronLogAdapter #88 #96

Merged
merged 13 commits into from
Mar 17, 2021
Merged

feat: add CronLogAdapter #88 #96

merged 13 commits into from
Mar 17, 2021

Conversation

GGXXLL
Copy link
Contributor

@GGXXLL GGXXLL commented Mar 17, 2021

No description provided.

@codecov
Copy link

codecov bot commented Mar 17, 2021

Codecov Report

Merging #96 (b5eefd4) into master (98e42c3) will increase coverage by 0.02%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #96      +/-   ##
==========================================
+ Coverage   75.93%   75.95%   +0.02%     
==========================================
  Files          74       75       +1     
  Lines        2863     2866       +3     
==========================================
+ Hits         2174     2177       +3     
  Misses        516      516              
  Partials      173      173              
Impacted Files Coverage Δ
cronopts/log.go 100.00% <100.00%> (ø)
otes/provider.go 81.08% <100.00%> (+0.25%) ⬆️
serve.go 87.64% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 98e42c3...b5eefd4. Read the comment docs.

otcron/log.go Outdated

// Error implements cron.Logger
func (r CronLogAdapter) Error(err error, msg string, keysAndValues ...interface{}) {
_ = level.Error(r.Logging).Log("msg", msg, "error", err, keysAndValues)
Copy link
Member

Choose a reason for hiding this comment

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

目前都是用err来代表error

otcron/log.go Outdated

// Info implements cron.Logger
func (r CronLogAdapter) Info(msg string, keysAndValues ...interface{}) {
_ = level.Info(r.Logging).Log("msg", msg, keysAndValues)
Copy link
Member

Choose a reason for hiding this comment

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

这里应该是有问题的。建议写个测试。

@Reasno Reasno requested review from lingwei0604 and nfangxu March 17, 2021 05:01
otcron/log.go Outdated
@@ -0,0 +1,21 @@
package otcron
Copy link
Member

Choose a reason for hiding this comment

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

这个换个名字吧,ot是opentracing的意思,这个和tracing没关系。。

Copy link
Member

@lingwei0604 lingwei0604 left a comment

Choose a reason for hiding this comment

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

其它我没意见。cc @nfangxu

cronopts/log.go Outdated
}

// Info implements cron.Logger
func (r CronLogAdapter) Info(msg string, keysAndValues ...interface{}) {
Copy link
Member

Choose a reason for hiding this comment

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

一般接收器变量的命名是接收器类型的第一个小写字母。

Reasno
Reasno previously approved these changes Mar 17, 2021
@Reasno Reasno merged commit a1a4b27 into DoNewsCode:master Mar 17, 2021
@GGXXLL GGXXLL deleted the develop branch March 17, 2021 10:14
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.

3 participants