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.

Sign up for GitHub

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

Jump to bottom

fix #1260 change middleware.Logger's default output #1336

Merged
merged 3 commits into from
Jul 18, 2019
Merged

fix #1260 change middleware.Logger's default output #1336

merged 3 commits into from
Jul 18, 2019

Conversation

nattawitc
Copy link
Contributor

Fixes #1260

  • change default logger to use context.Logger()
  • fix unit test: wrong assertion

@codecov
Copy link

codecov bot commented May 11, 2019 •
edited
Loading

Codecov Report

Merging #1336 into master will decrease coverage by 0.22%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1336      +/-   ##
==========================================
- Coverage   84.34%   84.11%   -0.23%     
==========================================
  Files          27       27              
  Lines        2012     2065      +53     
==========================================
+ Hits         1697     1737      +40     
- Misses        205      214       +9     
- Partials      110      114       +4
Impacted Files Coverage Δ
middleware/logger.go 87.61% <100%> (+2.15%) ⬆️
middleware/key_auth.go 67.79% <0%> (-3.64%) ⬇️
middleware/jwt.go 76.84% <0%> (-2.65%) ⬇️
bind.go 87.23% <0%> (-1.8%) ⬇️
echo.go 85.62% <0%> (ø) ⬆️
router.go 95.33% <0%> (+0.07%) ⬆️
response.go 71.05% <0%> (+1.6%) ⬆️

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 5d2c33a...af34c36. Read the comment docs.

@stale
Copy link

stale bot commented Jul 10, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix label Jul 10, 2019
vishr
vishr reviewed Jul 11, 2019
View reviewed changes
middleware/logger.go Outdated
@@ -74,7 +73,7 @@ var (
`"status":${status},"error":"${error}","latency":${latency},"latency_human":"${latency_human}"` +
`,"bytes_in":${bytes_in},"bytes_out":${bytes_out}}` + "\n",
CustomTimeFormat: "2006-01-02 15:04:05.00000",
Output: os.Stdout,
Output: nil,
Copy link
Member

Choose a reason for hiding this comment

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

You don't need to assign, it is nil by default.

@stale stale bot removed the wontfix label Jul 11, 2019
vishr
vishr approved these changes Jul 11, 2019
View reviewed changes
Copy link
Member

@vishr vishr left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Need to remove Output: assignment.

@vishr vishr self-requested a review July 11, 2019 17:51
vishr
vishr requested changes Jul 11, 2019
View reviewed changes
Copy link
Member

@vishr vishr left a comment

Choose a reason for hiding this comment

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

Please see the comments

@vishr vishr merged commit 8cfaf50 into labstack:master Jul 18, 2019
@nattawitc nattawitc deleted the middleware-logger branch July 18, 2019 04:37
@ccampbell ccampbell mentioned this pull request Oct 8, 2019
Closed
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vishr vishr vishr approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Shouldn't middleware.Logger's default output to be the same as echo.Logger
2 participants
@nattawitc @vishr