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: replace debug log with fmt package. #1560

Merged
merged 1 commit into from Sep 19, 2018
Merged

Conversation

appleboy
Copy link
Member

fix #829

remove log.SetFlags(0)

@appleboy appleboy self-assigned this Sep 19, 2018
@appleboy appleboy added this to the 1.4 milestone Sep 19, 2018
@codecov
Copy link

codecov bot commented Sep 19, 2018

Codecov Report

Merging #1560 into master will decrease coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1560      +/-   ##
==========================================
- Coverage   99.06%   99.05%   -0.01%     
==========================================
  Files          39       39              
  Lines        1915     1913       -2     
==========================================
- Hits         1897     1895       -2     
  Misses         14       14              
  Partials        4        4
Impacted Files Coverage Δ
debug.go 95% <100%> (-0.24%) ⬇️

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 b27b702...3d606cb. Read the comment docs.

4 similar comments
@codecov
Copy link

codecov bot commented Sep 19, 2018

Codecov Report

Merging #1560 into master will decrease coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1560      +/-   ##
==========================================
- Coverage   99.06%   99.05%   -0.01%     
==========================================
  Files          39       39              
  Lines        1915     1913       -2     
==========================================
- Hits         1897     1895       -2     
  Misses         14       14              
  Partials        4        4
Impacted Files Coverage Δ
debug.go 95% <100%> (-0.24%) ⬇️

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 b27b702...3d606cb. Read the comment docs.

@codecov
Copy link

codecov bot commented Sep 19, 2018

Codecov Report

Merging #1560 into master will decrease coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1560      +/-   ##
==========================================
- Coverage   99.06%   99.05%   -0.01%     
==========================================
  Files          39       39              
  Lines        1915     1913       -2     
==========================================
- Hits         1897     1895       -2     
  Misses         14       14              
  Partials        4        4
Impacted Files Coverage Δ
debug.go 95% <100%> (-0.24%) ⬇️

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 b27b702...3d606cb. Read the comment docs.

@codecov
Copy link

codecov bot commented Sep 19, 2018

Codecov Report

Merging #1560 into master will decrease coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1560      +/-   ##
==========================================
- Coverage   99.06%   99.05%   -0.01%     
==========================================
  Files          39       39              
  Lines        1915     1913       -2     
==========================================
- Hits         1897     1895       -2     
  Misses         14       14              
  Partials        4        4
Impacted Files Coverage Δ
debug.go 95% <100%> (-0.24%) ⬇️

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 b27b702...3d606cb. Read the comment docs.

@codecov
Copy link

codecov bot commented Sep 19, 2018

Codecov Report

Merging #1560 into master will decrease coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1560      +/-   ##
==========================================
- Coverage   99.06%   99.05%   -0.01%     
==========================================
  Files          39       39              
  Lines        1915     1913       -2     
==========================================
- Hits         1897     1895       -2     
  Misses         14       14              
  Partials        4        4
Impacted Files Coverage Δ
debug.go 95% <100%> (-0.24%) ⬇️

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 b27b702...3d606cb. Read the comment docs.

Copy link
Member

@thinkerou thinkerou left a comment

Choose a reason for hiding this comment

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

LGTM

@appleboy appleboy merged commit 07f1bf0 into gin-gonic:master Sep 19, 2018
@appleboy appleboy deleted the logs2 branch September 19, 2018 05:57
@philippgille
Copy link

@appleboy This leads to logging to stdout instead of stderr, right? That would be a big change. Is this wanted?

@appleboy
Copy link
Member Author

@philippgille just change the output from log to fmt package. you can see the changes of debug.go

@philippgille
Copy link

@appleboy: Yes I saw that, that's exactly what I mean. fmt prints to stdout, log prints to stderr.

Some people might in the past have piped stderr into file x and stdout into file y, now when updating gin the files don't contain their expected logs anymore, which is why it's a breaking change.

For access logs stdout makes sense. For errors, stderr might be a better fit. And users should be made aware of the change in Gin.

@philippgille
Copy link

@thinkerou what do you think about the change of general logging from stderr to stdout?

@giulianobr
Copy link

giulianobr commented Sep 25, 2018

Hey @appleboy, this change is breaking App Engine local development server to work properly. Is possible to revert to use the log instead of fmt ?

thanks

@appleboy
Copy link
Member Author

@giulianobr @philippgille Please help to review the new PR. #1571

appleboy added a commit that referenced this pull request Sep 27, 2018
fix #1560 changes are breaking in App Engine.

cc @giulianobr @philippgille
@appleboy
Copy link
Member Author

@philippgille #1571 already merged.

justinfx pushed a commit to justinfx/gin that referenced this pull request Nov 3, 2018
justinfx pushed a commit to justinfx/gin that referenced this pull request Nov 3, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

log.Println() does not print timestamp
4 participants