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

Improve custom logging #622

Closed
wants to merge 2 commits into from

Conversation

mpontillo
Copy link
Contributor

Closes: #621

@mpontillo mpontillo requested a review from a team as a code owner February 10, 2021 00:40
@pycak
Copy link

pycak commented Aug 13, 2021

I need this changes, what can I do to merge this PR?

Copy link
Member

@sio4 sio4 left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution!

The main concept of this PR is a kind of "exporter". You don't want to override the whole log() function (the behavior and the format of the function) but just want to use another custom exporter for printing out the same content in the same way. Is my understanding correct?

I agree that supporting various exporters could be a great idea! However, I think this kind of extended feature should be supported by external logging libraries and this is not in the scope of Pop's simple default logger. I would like to keep this PR in mind and will consider it in the future implementation of the logger, but for now, I am going to close this PR.

One more point. I think the current behavior, printing the logs to the stdout is not a good choice so I will fix it soon.

Thanks again!

(Just left some comments on the code for future reference)

logger.go Show resolved Hide resolved
logger.go Show resolved Hide resolved
@sio4 sio4 self-assigned this Sep 17, 2022
@sio4 sio4 added proposal A suggestion for a change, feature, enhancement, etc wontfix This will not be worked on labels Sep 17, 2022
@sio4 sio4 added this to the Proposal milestone Sep 17, 2022
@sio4 sio4 closed this Sep 17, 2022
@sio4 sio4 removed this from the Proposal milestone Sep 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
proposal A suggestion for a change, feature, enhancement, etc wontfix This will not be worked on
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Public pop.SetLogger is unusable because it accepts a private type
3 participants