Add support for messageFormat strings containing conditionals #442

timlohse1104 · 2023-07-23T20:25:05Z

See #441

It's now possible to pass --messageFormat option containing conditional statements for log data.

Example log data:
{level: 30, req: {id: 'foo'}}

Example --messageFormat containing conditional:
"{level} - {if req.id}({req.id}){end}{if msg} - {msg}{end}"

Example output:
"30 - (foo)"

Unescaped if / end block within --messageFormat will just be deleted.

coveralls · 2023-07-23T22:18:37Z

Pull Request Test Coverage Report for Build 5649830685

14 of 14 (100.0%) changed or added relevant lines in 1 file are covered.
No unchanged relevant lines lost coverage.
Overall coverage remained the same at 100.0%

Totals
Change from base Build 5620622122:	0.0%
Covered Lines:	419
Relevant Lines:	419

💛 - Coveralls

mcollina

Good work! Could you add some docs?

mcollina · 2023-07-24T08:19:24Z

test/lib/utils.internals.test.js

+  t.test('parseMessageFormat removes unescaped end statements', async t => {
+    const log = fastCopy(logData)
+    t.equal(internals.parseMessageFormat('{level} - {data1.data2}{end}', log), '{level} - {data1.data2}')
+  })


Could you add a few tests for not-matching if/else/end pair? What error would you get?

Yeah for sure. Thanks for the fast code review.

Added further test scenarios.

…ition and property key and add further unit tests for parseMessageFormat function

timlohse1104 · 2023-07-24T09:31:20Z

Updated documentation with --messageFormat condition example.
Add matching for condition and property keys.
Add further unit tests for parseMessageFormat function.

Readme.md

Co-authored-by: Matteo Collina <matteo.collina@gmail.com>

jsumners · 2023-07-24T12:45:04Z

lib/utils.js

+function parseMessageFormat (messageFormat, log) {
+  messageFormat = messageFormat.replace(/{if (.*?)}(.*?){end}/g, function (_, key, value) {
+    const propertyValue = getPropertyValue(log, key)
+    if (propertyValue && value.includes(key)) {
+      return value.replace(new RegExp('{' + key + '}', 'g'), propertyValue)
+    } else {
+      return ''
+    }
+  })
+
+  // Remove unescaped if blocks
+  messageFormat = messageFormat.replace(/{if (.*?)}/g, '')
+  // Remove unescaped end blocks
+  messageFormat = messageFormat.replace(/{end}/g, '')
+
+  return messageFormat.replace(/\s+/g, ' ').trim()
+}


I think this would be a bit easier to read:

Suggested change

function parseMessageFormat (messageFormat, log) {

messageFormat = messageFormat.replace(/{if (.*?)}(.*?){end}/g, function (_, key, value) {

const propertyValue = getPropertyValue(log, key)

if (propertyValue && value.includes(key)) {

return value.replace(new RegExp('{' + key + '}', 'g'), propertyValue)

} else {

return ''

}

})

// Remove unescaped if blocks

messageFormat = messageFormat.replace(/{if (.*?)}/g, '')

// Remove unescaped end blocks

messageFormat = messageFormat.replace(/{end}/g, '')

return messageFormat.replace(/\s+/g, ' ').trim()

}

function parseMessageFormat (messageFormat, log) {

messageFormat = messageFormat.replace(/{if (.*?)}(.*?){end}/g, replacer)

// Remove unescaped if blocks

messageFormat = messageFormat.replace(/{if (.*?)}/g, '')

// Remove unescaped end blocks

messageFormat = messageFormat.replace(/{end}/g, '')

return messageFormat.replace(/\s+/g, ' ').trim()

function replacer (_, key, value) {

const propertyValue = getPropertyValue(log, key)

if (propertyValue && value.includes(key)) {

return value.replace(new RegExp('{' + key + '}', 'g'), propertyValue)

} else {

return ''

}

}

}

Updated accordingly.

jsumners · 2023-07-24T12:47:46Z

test/lib/utils.internals.test.js

+    t.equal(internals.parseMessageFormat('{level} - {if data1.data2}{data1.data2} ({msg}){end}', log), '{level} - bar ({msg})')
+  })
+
+  t.test('parseMessageFormat removes unescaped if statements', async t => {


I believe you mean non-terminated instead of unescaped?

Changed wording.

jsumners · 2023-07-24T12:48:46Z

test/lib/utils.internals.test.js

+    t.equal(internals.parseMessageFormat('{level} - {if data1.data2}{data1.data2}', log), '{level} - {data1.data2}')
+  })
+
+  t.test('parseMessageFormat removes unescaped end statements', async t => {


I think "floating" is a better term than "unescaped" here.

Changed wording.

jsumners · 2023-07-24T12:50:20Z

test/lib/utils.internals.test.js

+    t.equal(internals.parseMessageFormat('{level} - {if data1.data2}{data1.data2}', log), '{level} - {data1.data2}')
+  })
+
+  t.test('parseMessageFormat removes unescaped end statements', async t => {


What happens if the format string is?:

{level} - {if foo}something{end}{end}

(i.e. someone tried to add an unsupported nested condition but failed to add the leading if)

The additional {end} within the if / end statement will be removed. Added test scenario for this.

jsumners · 2023-07-24T12:52:28Z

lib/utils.js

+ *
+ * @returns {string} The parsed messageFormat.
+ */
+function parseMessageFormat (messageFormat, log) {


I think this would be a better name:

Suggested change

function parseMessageFormat (messageFormat, log) {

function interpretConditionals (messageFormat, log) {

Renamed accordingly.

timlohse1104 · 2023-07-24T13:03:50Z

Hey @jsumners thanks for the code review. I will look into it after work. Please excuse my poor english var and function wording. Will edit.

jsumners · 2023-07-24T13:08:48Z

Hey @jsumners thanks for the code review. I will look into it after work. Please excuse my poor english var and function wording. Will edit.

No problem. The review phase is there to get us into an agreeable state.

…teral unit test scenario, refactored interpretConditionals logic to be more readable and renamed internal unit tests to be more fitting

lib/utils.js

Co-authored-by: James Sumners <321201+jsumners@users.noreply.github.com>

mcollina

lgtm

jsumners

Looks good to me.

[![Mend Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com) This PR contains the following updates: | Package | Change | Age | Adoption | Passing | Confidence | |---|---|---|---|---|---| | [pino-pretty](https://togithub.com/pinojs/pino-pretty) | [`^10.1.0` -> `^10.2.0`](https://renovatebot.com/diffs/npm/pino-pretty/10.1.0/10.2.0) | [![age](https://developer.mend.io/api/mc/badges/age/npm/pino-pretty/10.2.0?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![adoption](https://developer.mend.io/api/mc/badges/adoption/npm/pino-pretty/10.2.0?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![passing](https://developer.mend.io/api/mc/badges/compatibility/npm/pino-pretty/10.1.0/10.2.0?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![confidence](https://developer.mend.io/api/mc/badges/confidence/npm/pino-pretty/10.1.0/10.2.0?slim=true)](https://docs.renovatebot.com/merge-confidence/) | --- ### Release Notes <details> <summary>pinojs/pino-pretty (pino-pretty)</summary> ### [`v10.2.0`](https://togithub.com/pinojs/pino-pretty/releases/tag/v10.2.0) [Compare Source](https://togithub.com/pinojs/pino-pretty/compare/v10.1.0...v10.2.0) #### What's Changed - Remove coveralls by [@jsumners](https://togithub.com/jsumners) in [https://github.com/pinojs/pino-pretty/pull/443](https://togithub.com/pinojs/pino-pretty/pull/443) - Add support for messageFormat strings containing conditionals by [@timlohse1104](https://togithub.com/timlohse1104) in [https://github.com/pinojs/pino-pretty/pull/442](https://togithub.com/pinojs/pino-pretty/pull/442) #### New Contributors - [@timlohse1104](https://togithub.com/timlohse1104) made their first contribution in [https://github.com/pinojs/pino-pretty/pull/442](https://togithub.com/pinojs/pino-pretty/pull/442) **Full Changelog**: pinojs/pino-pretty@v10.1.0...v10.2.0 </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Enabled. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about these updates again. --- - [ ] If you want to rebase/retry this PR, check this box --- This PR has been generated by [Mend Renovate](https://www.mend.io/free-developer-tools/renovate/). View repository job log [here](https://developer.mend.io/github/fwouts/previewjs).  Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

timlohse1104 added 2 commits July 23, 2023 21:16

Add working happy path for if / end block conditions

39feab1

Refactor parsing of messageFormat conditionals in a separate function

2acc0fe

timlohse1104 mentioned this pull request Jul 23, 2023

Extention for --messageFormat option to support go template control structures #441

Closed

mcollina requested changes Jul 24, 2023

View reviewed changes

Add documentation for conditions in readme, add matching between cond…

78578c5

…ition and property key and add further unit tests for parseMessageFormat function

timlohse1104 requested a review from mcollina July 24, 2023 09:32

mcollina reviewed Jul 24, 2023

View reviewed changes

Readme.md Outdated Show resolved Hide resolved

Update docs of messageFormat with more fitting example

b6894c6

Co-authored-by: Matteo Collina <matteo.collina@gmail.com>

timlohse1104 requested a review from mcollina July 24, 2023 09:57

jsumners requested changes Jul 24, 2023

View reviewed changes

timlohse1104 added 2 commits July 24, 2023 17:01

Renamed parseMessageFormat into interpretConditionals, add further in…

8f22437

…teral unit test scenario, refactored interpretConditionals logic to be more readable and renamed internal unit tests to be more fitting

Merge branch 'master' of github.com:timlohse1104/pino-pretty

25eab67

timlohse1104 requested a review from jsumners July 24, 2023 15:06

jsumners reviewed Jul 24, 2023

View reviewed changes

lib/utils.js Outdated Show resolved Hide resolved

Renamed comments to be more fitting

44ac4c3

Co-authored-by: James Sumners <321201+jsumners@users.noreply.github.com>

timlohse1104 requested a review from jsumners July 24, 2023 21:08

mcollina approved these changes Jul 25, 2023

View reviewed changes

jsumners approved these changes Jul 25, 2023

View reviewed changes

jsumners merged commit ff7b238 into pinojs:master Jul 25, 2023
6 checks passed

hpbonfim mentioned this pull request Aug 20, 2024

[Snyk] Upgrade pino-pretty from 10.2.0 to 10.3.1 hpbonfim/semana-javascript-expert08#2

Open

IT21071652 mentioned this pull request Sep 26, 2024

[Snyk] Upgrade pino-pretty from 10.0.1 to 10.3.1 IT21071652/Educode-web-Application#5

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for messageFormat strings containing conditionals #442

Add support for messageFormat strings containing conditionals #442

timlohse1104 commented Jul 23, 2023 •

edited

Loading

coveralls commented Jul 23, 2023 •

edited

Loading

mcollina left a comment

mcollina Jul 24, 2023

timlohse1104 Jul 24, 2023

timlohse1104 Jul 24, 2023

timlohse1104 commented Jul 24, 2023

jsumners Jul 24, 2023

timlohse1104 Jul 24, 2023

jsumners Jul 24, 2023

timlohse1104 Jul 24, 2023

jsumners Jul 24, 2023

timlohse1104 Jul 24, 2023

jsumners Jul 24, 2023

timlohse1104 Jul 24, 2023

jsumners Jul 24, 2023

timlohse1104 Jul 24, 2023

timlohse1104 commented Jul 24, 2023

jsumners commented Jul 24, 2023

mcollina left a comment

jsumners left a comment

	function parseMessageFormat (messageFormat, log) {
	function interpretConditionals (messageFormat, log) {

Add support for messageFormat strings containing conditionals #442

Add support for messageFormat strings containing conditionals #442

Conversation

timlohse1104 commented Jul 23, 2023 • edited Loading

See #441

coveralls commented Jul 23, 2023 • edited Loading

Pull Request Test Coverage Report for Build 5649830685

💛 - Coveralls

mcollina left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timlohse1104 commented Jul 24, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timlohse1104 commented Jul 24, 2023

jsumners commented Jul 24, 2023

mcollina left a comment

Choose a reason for hiding this comment

jsumners left a comment

Choose a reason for hiding this comment

timlohse1104 commented Jul 23, 2023 •

edited

Loading

coveralls commented Jul 23, 2023 •

edited

Loading