Logger refactor: improvements and more polishing - #13
Conversation
EmilianoSanchez
changed the title
| ERROR: 4, | ||
| NONE: 5 | ||
| }; | ||
|
|
There was a problem hiding this comment.
Given the fact that we are using TS, you could implement this Object as a const enum
There was a problem hiding this comment.
There was a problem hiding this comment.
It is a great idea using const enums, but after trying it, found a few places where I needed to access enum values dynamically and was not possible to use it.
| const formattedText = this._generateLogMessage(level, msg); | ||
|
|
||
| console.log(formattedText); | ||
| _log(level: LogLevel, msg: string | number, args?: any[]) { |
There was a problem hiding this comment.
Since it's not private anymore, is there any reason to use the _? (I'm assuming we use the convention of adding the underscore only for private vars)
There was a problem hiding this comment.
you are right. putting back into private. we don't want to access it directly, accept for a special single case during validation where we want to force a log message no matter the log level.
| if (format) msg = _sprintf(format, args); | ||
| else msg = `Message code ${msg}${args ? ', with args: ' + args.toString() : ''}`; |
There was a problem hiding this comment.
You can use Ternary operator here to avoid the if else statement:
msg = format ? _sprintf(format, args) : `Message code ${msg}${args ? ', with args: ' + args.toString() : ''}`;
| function sprintf(format: string = '', args: any[] = []): string { | ||
| // exported for testing purposes only | ||
| export function _sprintf(format: string = '', args: any[] = []): string { | ||
| var i = 0; |
There was a problem hiding this comment.
Use let instead of var
| } else if (typeof debugValue === 'string' && isLogLevelString(debugValue)) { | ||
| return debugValue; | ||
| } else { | ||
| return undefined; |
There was a problem hiding this comment.
Is there an scenario where undefined is a valid case? If not, maybe the 'last resort' return should be the NONE logLevel (I may be missing a lot of context, but usually you try to avoid returning an undefined value).
There was a problem hiding this comment.
returning undefined is the way the getLogLevel "validator" tell us that the provided input (debugValue param) is invalid.
Other validators, in the case of invalid input, either: (a) logs a message and return a default value, or (b) throws an error. In the getLogLevel case, the former was not possible (it doesn't have the logger instance to log the error), and the later I think is overkill in terms of extra code.
2 participants
Javascript commons library
What did you accomplish?
Changes:
How do we test the changes introduced in this PR?
Added UTs.
Extra Notes