-
Notifications
You must be signed in to change notification settings - Fork 151
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
Add support to hide metadata in log line #445
Changes from all commits
39feab1
2acc0fe
78578c5
b6894c6
8f22437
25eab67
44ac4c3
5e4f0f4
d2d0f45
8a4066f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -49,7 +49,8 @@ const defaultOptions = { | |
hideObject: false, | ||
ignore: 'hostname', | ||
include: undefined, | ||
singleLine: false | ||
singleLine: false, | ||
hideMetadata: false | ||
} | ||
|
||
function prettyFactory (options) { | ||
|
@@ -127,39 +128,10 @@ function prettyFactory (options) { | |
log = filterLog({ log, ignoreKeys, includeKeys }) | ||
} | ||
|
||
const prettifiedLevel = prettifyLevel({ log, colorizer, levelKey, prettifier: customPrettifiers.level, ...customProps }) | ||
const prettifiedMetadata = prettifyMetadata({ log, prettifiers: customPrettifiers }) | ||
const prettifiedTime = prettifyTime({ log, translateFormat: opts.translateTime, timestampKey, prettifier: customPrettifiers.time }) | ||
|
||
let line = '' | ||
if (opts.levelFirst && prettifiedLevel) { | ||
line = `${prettifiedLevel}` | ||
} | ||
|
||
if (prettifiedTime && line === '') { | ||
line = `${prettifiedTime}` | ||
} else if (prettifiedTime) { | ||
line = `${line} ${prettifiedTime}` | ||
} | ||
|
||
if (!opts.levelFirst && prettifiedLevel) { | ||
if (line.length > 0) { | ||
line = `${line} ${prettifiedLevel}` | ||
} else { | ||
line = prettifiedLevel | ||
} | ||
} | ||
|
||
if (prettifiedMetadata) { | ||
if (line.length > 0) { | ||
line = `${line} ${prettifiedMetadata}:` | ||
} else { | ||
line = prettifiedMetadata | ||
} | ||
} | ||
|
||
if (line.endsWith(':') === false && line !== '') { | ||
line += ':' | ||
if (!opts.hideMetadata) { | ||
line += createLineMetadata() | ||
} | ||
|
||
if (prettifiedMessage !== undefined) { | ||
|
@@ -206,6 +178,46 @@ function prettyFactory (options) { | |
} | ||
|
||
return line | ||
|
||
function createLineMetadata () { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does this function need to be defined as an inner function of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We could put it in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My concern is legibility and maintainability. The inlined function is easier to read than the nested conditional version, but it is still a lot of code deeply nested within another function. I think the function name is fine. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @timlohse1104 I think you'll find it much easier to accommodate the ask now that #453 has been merged. Unfortunately, it'll be easier to start with a fresh PR. Rebasing this one may be too difficult. If you have any questions, don't hesitate to ask. |
||
let lineMetadata = '' | ||
|
||
const prettifiedLevel = prettifyLevel({ log, colorizer, levelKey, prettifier: customPrettifiers.level, ...customProps }) | ||
const prettifiedMetadata = prettifyMetadata({ log, prettifiers: customPrettifiers }) | ||
const prettifiedTime = prettifyTime({ log, translateFormat: opts.translateTime, timestampKey, prettifier: customPrettifiers.time }) | ||
|
||
if (opts.levelFirst && prettifiedLevel) { | ||
lineMetadata = `${prettifiedLevel}` | ||
} | ||
|
||
if (prettifiedTime && lineMetadata === '') { | ||
lineMetadata = `${prettifiedTime}` | ||
} else if (prettifiedTime) { | ||
lineMetadata = `${lineMetadata} ${prettifiedTime}` | ||
} | ||
|
||
if (!opts.levelFirst && prettifiedLevel) { | ||
if (lineMetadata.length > 0) { | ||
lineMetadata = `${lineMetadata} ${prettifiedLevel}` | ||
} else { | ||
lineMetadata = prettifiedLevel | ||
} | ||
} | ||
|
||
if (prettifiedMetadata) { | ||
if (lineMetadata.length > 0) { | ||
lineMetadata = `${lineMetadata} ${prettifiedMetadata}:` | ||
} else { | ||
lineMetadata = prettifiedMetadata | ||
} | ||
} | ||
|
||
if (lineMetadata.endsWith(':') === false && lineMetadata !== '') { | ||
lineMetadata += ':' | ||
} | ||
|
||
return lineMetadata | ||
} | ||
} | ||
} | ||
|
||
|
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.
Can this be done in some other way than continually nesting the core code of this function further and further to the right?
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.
Do you have something in mind. I could see some options. One with higher refactoring effort like using strategy pattern (https://refactoring.guru/design-patterns/strategy) and some with lower effort like using switch statements/ lookup tables.
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.
I think a simple approach would be to move the now optional code path into a function. Note, such a function should return a string, not mutate one through a side effect.
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.
Added an approach