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 new logging framework consistently #1459

Conversation

MarcelRobitaille
Copy link
Collaborator

Replace all instances of console.log and alternatives with this.$log.info or Vue.$log.info.

In #1404, I noticed that although a logging framework was added in #1283, console.log and console.error were still used frequently throughout the app. This PR aims to replace all of these with the logging framework.

Signed-off-by: Marcel Robitaille <mail@marcelrobitaille.me>
@MarcelRobitaille
Copy link
Collaborator Author

Some console.logs are inside if (this.verboseDebugLogging). Can this logic be removed now that #1283 has functionality to enable and disable logging?

@github-actions
Copy link

github-actions bot commented Jan 22, 2023

Test Results

     21 files     959 suites   5m 15s ⏱️
   498 tests    498 ✔️ 0 💤 0
3 486 runs  3 485 ✔️ 1 💤 0

Results for commit 64c3295.

♻️ This comment has been updated with latest results.

Comment on lines 49 to 53
/* This is left here as an example in case the routes need to be debugged again
'$route' (to, from) {
console.log(this.$window.isSameBaseRoute(from.fullPath, to.fullPath))
this.$log.debug(this.$window.isSameBaseRoute(from.fullPath, to.fullPath))
},
*/
Copy link
Collaborator

Choose a reason for hiding this comment

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

These lines are commented out. After it is redirected to a debug-level logger, we could remove the comment and just let the logger ignore the logged data in the normal (non-debug) case. Am I right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure. Sounds like a plan.

@christianlupus
Copy link
Collaborator

Some console.logs are inside if (this.verboseDebugLogging). Can this logic be removed now that #1283 has functionality to enable and disable logging?

Yes, I would say so, you can switch to the new logger with debugging level. This seems a good idea.

A general question: What do you consider needed to finish this PR?

@MarcelRobitaille
Copy link
Collaborator Author

I was just waiting for your feedback on how to handle verboseDebugLogging

Now, we don't need to comment them out, we can just change the log level

Signed-off-by: Marcel Robitaille <mail@marcelrobitaille.me>
Instead, just do everything through the logging API

Signed-off-by: Marcel Robitaille <mail@marcelrobitaille.me>
@MarcelRobitaille MarcelRobitaille marked this pull request as ready for review March 18, 2023 00:18
@christianlupus
Copy link
Collaborator

Ahh, ok. Sorry for the delay.

@MarcelRobitaille
Copy link
Collaborator Author

No worries. I've been super busy too.

Signed-off-by: Christian Wolf <github@christianwolf.email>
@christianlupus christianlupus merged commit 0f695b2 into nextcloud:master Mar 24, 2023
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.

2 participants