-
Notifications
You must be signed in to change notification settings - Fork 120
Move logging APIs (and defaults) to mircore #4644
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
Conversation
mattkae
left a comment
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.
One request!
| geometry/rectangles.cpp | ||
| input/mousekeys_keymap.cpp | ||
| log.cpp | ||
| logging/dumb_console_logger.cpp |
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.
Before we land this in the public API, can we change this name to something a little more informative? Perhaps StdOutConsoleLogger or something similar?
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.
Isn't StdOut both wrong (doesn't mention stderr) and tautologous?
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.
FWIW I'll also defend the Dumb: that is a comparison to a Smart logger that enabled things like filtering based on level and component. (Which something we're talking about doing now.)
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.
Maybe Basic or Simple is better, but I'm fine either way 👍
RAOF
left a comment
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.
It seems likely that we'll be changing (or perhaps deprecating parts of) this API as a part of the logging work coming up.
Should we wait until we've decide what logging API additions/deprecations we are going to make before landing something in mircore we might almost immediately deprecate?
I have been wondering the same thing. But, from what I've seen of the logging API discussion, we can implement this API in terms of the proposed solution (which is an easy migration path). The main change that seems likely is |
TICS Quality Gate✔️ PassedNo changed files applicable for TICS analysis quality gating. TICS / TICS / Run TICS analysis |
mattkae
left a comment
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.
Nothing blocking on my end, just a nit on the existing name
The logging APIs are stable and useful to downstream projects. Make them more easily available