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

Use Logger in restapi.go #1101

Closed
wants to merge 1 commit into from
Closed

Conversation

dschmidt
Copy link

@dschmidt dschmidt commented Feb 20, 2022

There are way more occurences of log.Print(...) in restapi.go

I'm just not sure how to go about for fixing those. They are not so important because they are hidden behind the Session.Debug flag anyway. They are basically for printing out whole http requests to the console.

I played around with introducing a new LogLevel LogTrace and not using the Session.Debug flag for deciding, whether those messages should be logged or not.
We could keep Session.Debug for api compatibility, but it would still be a behavioral change ... so not sure, what you would like there.

@FedorLap2006
Copy link
Collaborator

I think would be better to use Debug here. Since you probably won't really want to have this in error logs, unless if you're in Debug mode.

@dschmidt
Copy link
Author

Changed

@dschmidt
Copy link
Author

s.Debug has a comment that says it's meant to go away ... in the end I don't care how we do it, I just want to get rid of this unconditional output on stdout.

By now I feel it might be easier, if you just push a fix yourself, as it's just a one liner anyway? :)

@FedorLap2006
Copy link
Collaborator

s.Debug has a comment that says it's meant to go away ... in the end I don't care how we do it, I just want to get rid of this unconditional output on stdout.

By now I feel it might be easier, if you just push a fix yourself, as it's just a one liner anyway? :)

Yeah, I think the best solution here is to use s.Debug, and just put a TODO to replace it to s.log(LogDebug, ...) or s.log(LogError, ...) in the future

@FedorLap2006
Copy link
Collaborator

After a bit of thinking, I think I'll close this in favor of #1043.

@dschmidt
Copy link
Author

fine with me, happy we got rid of the output - thanks!

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.

None yet

2 participants