-
Notifications
You must be signed in to change notification settings - Fork 521
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
Fixes part of #1433: Rename the logger class to consoleLogger #1434
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Just one suggestion, shouldn't we rename the variable name to consoleLogger
as well ? I think It would make files more readable in cases where multiple logging scenarios(console/event/exception) exist in the same file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, just one thing to confirm this is updated with our yesterday's ktlint app module check merge.
The whole goal is to remove multiple logging scenarios :) Hence I am not changing the name. Eventually we will be replacing the consoleLogger with OppiaLogger in the app and domain modules, so I dont want to change variable names right now |
Yep I updated the branch |
Explanation
With the new forms of firebase logging coming up, we would like app/domain module files to use all logging functionality from a single logger. This PR starts the work of it. See the issue for details on how we intend to restructure it.
Fixes part of #1433
Checklist