-
Notifications
You must be signed in to change notification settings - Fork 52
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
Added option to turn off logger #2366
Conversation
Signed-off-by: avifenesh <aviarchi1994@gmail.com>
76e0e4c
to
6f8c2c3
Compare
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.
question inside
@@ -14,7 +14,8 @@ public enum Level | |||
Warn = 1, | |||
Info = 2, | |||
Debug = 3, | |||
Trace = 4 | |||
Trace = 4, | |||
Off = 5, |
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.
nit: I think it's more common to use "Disabled"
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.
This is the built-in naming of the rust logging library we use, I want to keep it consistent
@@ -28,13 +28,13 @@ | |||
public final class Logger { | |||
@Getter | |||
public enum Level { | |||
DISABLED(-2), |
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.
why changing it and not using the existing code? setting disabled with -2 allows you to use if (!(level.getLevel() <= loggerLevel.getLevel())) {
in the log
function without checking specifically if logger.Off
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.
In the log, it was already existing the specific check.
In the other, I don't want to check, and I want it to be consistent over all clients. “Special” implantation for one language are not a thing we do. The disabled in java only was supposed to be blocked when it was created so it is implemented for all clients, as I do now, even if the request to fix is for ts only.
Adding a negative number will bring major changes to all other clients, and I think consistency is better than one less if.
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.
And about naming, the tracing crate we use, use off so I want to be consistent.
if (level == Level.DISABLED) { | ||
if (level == Level.OFF) { |
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.
see my comment above
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.
As said, it was already there as you see, but even if not, answered above.
f84f03e
to
a85c806
Compare
Signed-off-by: avifenesh <aviarchi1994@gmail.com>
a85c806
to
95a27ef
Compare
* Added option to turn off logger Signed-off-by: avifenesh <aviarchi1994@gmail.com> * remove mistaknlly changed util logging Signed-off-by: avifenesh <aviarchi1994@gmail.com> --------- Signed-off-by: avifenesh <aviarchi1994@gmail.com>
Added OFF option to logger