Skip to content

Conversation

@fclairamb
Copy link
Owner

@fclairamb fclairamb commented Oct 7, 2017

  • Switched logging from log15 to go-kit/log with logging level
    • Switched from a global logger to a passed & shared one
    • Using contextualized logging instead of repeating the clientId
  • Removed capitalized errors

- Switched logging from log15 to go-kit/log with logging level
-- Switched from a global logger to a passed & shared one
-- Using contextualized logging instead of repeating the "clientId"
- Removed capitalized errors
- Switched logging from log15 to go-kit/log with logging level
-- Switched from a global logger to a passed & shared one
-- Using contextualized logging instead of repeating the "clientId"
- Removed capitalized errors
@fclairamb fclairamb requested review from asv, byrnedo and mgenov October 7, 2017 21:50
@fclairamb fclairamb self-assigned this Oct 7, 2017
Copy link
Collaborator

@mgenov mgenov left a comment

Choose a reason for hiding this comment

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

LGTM !

server/server.go Outdated
}

log15.Info("Starting...")
level.Info(server.Logger).Log("msg", "Starting...", "action", "ftp.starting")
Copy link
Collaborator

Choose a reason for hiding this comment

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

As "msg" is duplicated on several places it could be extracted as package level constant. It won't remove the duplication actually, but will decouple name from code.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Yep, that's true. It doesn't make it smaller / more manageable but easier to track and manage in an IDE. That's worth trying anyway.

Copy link
Collaborator

@asv asv left a comment

Choose a reason for hiding this comment

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

LGTM

@fclairamb fclairamb merged commit 6280984 into master Oct 9, 2017
@fclairamb fclairamb deleted the switch-to-gokit branch October 10, 2017 23:21
@fclairamb fclairamb mentioned this pull request Oct 11, 2017
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.

4 participants