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

Refactor logger.New to accept FieldLogger #824

Merged
merged 3 commits into from Mar 13, 2023
Merged

Refactor logger.New to accept FieldLogger #824

merged 3 commits into from Mar 13, 2023

Conversation

inancgumus
Copy link
Member

@inancgumus inancgumus commented Mar 10, 2023 •
edited

While doing this I've also refactored the code a little bit. Let me know if you think the otherwise.

Closes: #823

@inancgumus inancgumus self-assigned this Mar 10, 2023
@inancgumus inancgumus force-pushed the refactor/823-logrus-fieldlogger-hack branch from 95bfbde to d70de4a Compare March 10, 2023 13:33
@inancgumus
Refactor logger.New to accept FieldLogger
171cc76 
While doing this I've also refactored the code a little bit. Let me know
if you think the otherwise.

Closes: #823
@inancgumus inancgumus force-pushed the refactor/823-logrus-fieldlogger-hack branch from d70de4a to 171cc76 Compare March 10, 2023 13:39
@inancgumus inancgumus marked this pull request as ready for review March 10, 2023 13:43
ankur22
ankur22 reviewed Mar 10, 2023
View reviewed changes
log/logger.go Outdated Show resolved Hide resolved
inancgumus added a commit that referenced this pull request Mar 10, 2023
@inancgumus
fixup! always use a logger no matter what
3b2de1b 
resolves: #824 (comment)
@inancgumus inancgumus requested a review from ankur22 March 10, 2023 16:05
@inancgumus inancgumus force-pushed the refactor/823-logrus-fieldlogger-hack branch from 3b2de1b to c5b1746 Compare March 10, 2023 16:08
ankur22
ankur22 reviewed Mar 10, 2023
View reviewed changes
log/logger.go Outdated Show resolved Hide resolved
@inancgumus inancgumus requested a review from ankur22 March 10, 2023 16:13
ankur22
ankur22 approved these changes Mar 10, 2023
View reviewed changes
Copy link
Collaborator

@ankur22 ankur22 left a comment

Choose a reason for hiding this comment

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

LGTM

@inancgumus
Copy link
Member Author

inancgumus commented Mar 10, 2023 •
edited

I forgot to respect @ka3de's previous comment, and now sent this commit to comply: 7d0c6a3

@ka3de wrote:
Comply with our logging API to produce the log (which allows to specify a "category")

@inancgumus inancgumus requested a review from ankur22 March 10, 2023 16:30
ka3de
ka3de approved these changes Mar 13, 2023
View reviewed changes
Copy link
Collaborator

@ka3de ka3de left a comment

Choose a reason for hiding this comment

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

LGTM.

@inancgumus inancgumus merged commit ef2ed4d into main Mar 13, 2023
@inancgumus inancgumus deleted the refactor/823-logrus-fieldlogger-hack branch March 13, 2023 07:51
@inancgumus inancgumus removed the request for review from na-- March 13, 2023 07:51
@ka3de ka3de mentioned this pull request Apr 3, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ankur22 ankur22 ankur22 approved these changes

@ka3de ka3de ka3de approved these changes

Assignees

@inancgumus inancgumus

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Allow making a logger from FieldLogger instead of concrete *Logrus
3 participants
@inancgumus @ankur22 @ka3de