Add EscapeHandling parameter in ConvertTo-Json cmdlet by iSazonov · Pull Request #7775 · PowerShell/PowerShell

iSazonov · 2018-09-13T07:08:36Z

PR Summary

Add EscapeHandling parameter in ConvertTo-Json cmdlet to allow escape the HTML (<, >, &, ', ") and control characters (e.g. newline).

PR Checklist

iSazonov · 2018-09-13T07:11:34Z

/cc @PlagueHO @mkht @markekraus

PlagueHO · 2018-09-13T07:28:35Z

Great solution - thanks @iSazonov - really appreciate it. 🏆

iSazonov · 2018-09-13T10:09:08Z

@TravisEz13 Should CI Travis be removed?

TravisEz13 · 2018-09-13T20:33:51Z

@iSazonov Yes, it is now replaced with the PowerShell-CI-linux and PowerShell-CI-macos

SteveL-MSFT · 2018-09-14T22:41:33Z

test/powershell/Modules/Microsoft.PowerShell.Utility/ConvertTo-Json.Tests.ps1

Turn this into -TestCases? Benefit is if first assert fails, the whole thing doesn't fail.

SteveL-MSFT · 2018-09-17T16:44:14Z

test/powershell/Modules/Microsoft.PowerShell.Utility/ConvertTo-Json.Tests.ps1

Is there a reason for this change from using the verbose channel to know that the cmdlet has started to a sleep? On a particularly slow container/VM, it might take longer than 300ms.

We removed the verbose message in #7487 and currently the test take 10 seconds until timeout.
We could consider adding debug message instead of the verbose message.

@iSazonov ok, got it. Seems like the right thing to do would be to have some test hook so that we can make this predictable as anything with small timeouts are more likely to fail in CI especially when it's under load.

I added verbose in BeginProcessing(). (Previously it was in ProcessRecord())

SteveL-MSFT · 2018-09-19T16:43:06Z

test/powershell/Modules/Microsoft.PowerShell.Utility/ConvertTo-Json.Tests.ps1

Per comment on line 33, Stop() is synchronous and in the chance it blocks, Pester is blocked. Perhaps we can change the sleep on line 34 to Wait-UntilTrue where State changes?

I think we could discuss this in #7819 and perhaps remove the tests at all.

If you can revert changes to this test in this PR, I think we can move forward with this PR and resolve this test in the other PR.

Reverted in the PR and moved to #7819.

iSazonov · 2018-09-20T08:53:47Z

Reopen to restart PowerShell-CI-Linux.

SteveL-MSFT · 2018-09-21T17:12:50Z

test/powershell/Modules/Microsoft.PowerShell.Utility/ConvertTo-Json.Tests.ps1

            $obj = [PSCustomObject]@{P1 = ''; P2 = ''; P3 = ''; P4 = ''; P5 = ''; P6 = ''}
            $obj.P1 = $obj.P2 = $obj.P3 = $obj.P4 = $obj.P5 = $obj.P6 = $obj
-            1..100 | Foreach-Object { $obj } | ConvertTo-Json -Depth 10 -Verbose
+            1..100 | Foreach-Object { $obj } | ConvertTo-Json -Depth 10


Surprised this test passed if you remove -verbose as line 35 is waiting on verbose output?

The waiting take 10 seconds until timeout.

You are removing -Verbose because another PR removed the verbose message (which you added back with the test hook in the other PR), right? If so, I suppose this is ok, but it seems this change doesn't have any impact to this test so wondering why have this change in this PR at all?

Reverted the change.

SteveL-MSFT · 2018-09-24T17:42:14Z

test/powershell/Modules/Microsoft.PowerShell.Utility/ConvertTo-Json.Tests.ps1

            $obj = [PSCustomObject]@{P1 = ''; P2 = ''; P3 = ''; P4 = ''; P5 = ''; P6 = ''}
            $obj.P1 = $obj.P2 = $obj.P3 = $obj.P4 = $obj.P5 = $obj.P6 = $obj
-            1..100 | Foreach-Object { $obj } | ConvertTo-Json -Depth 10 -Verbose
+            1..100 | Foreach-Object { $obj } | ConvertTo-Json -Depth 10


You are removing -Verbose because another PR removed the verbose message (which you added back with the test hook in the other PR), right? If so, I suppose this is ok, but it seems this change doesn't have any impact to this test so wondering why have this change in this PR at all?

iSazonov · 2018-09-26T05:15:27Z

@daxian-dbw @dantraMSFT Could you please do (fast) review before we merge?

markekraus

LGTM

Add EscapeHandling parameter in ConvertTo-Json cmdlet to allow escape the HTML (<, >, &, ', ") and control characters (e.g. newline).

iSazonov requested review from JamesWTruher, PaulHigin and dantraMSFT as code owners September 13, 2018 07:08

iSazonov added the Documentation Needed in this repo Documentation is needed in this repo label Sep 13, 2018

iSazonov self-assigned this Sep 13, 2018

iSazonov requested a review from SteveL-MSFT September 13, 2018 07:11

iSazonov force-pushed the convertto-json-escapehandling branch 2 times, most recently from d2e6719 to 1af310d Compare September 13, 2018 09:39

SteveL-MSFT requested changes Sep 14, 2018

View reviewed changes

iSazonov added 3 commits September 17, 2018 16:24

Add EscapeHandling parameter in ConvertTo-Json cmdlet

4f866bd

Fix test

9a1feae

Split tests

488fd3c

iSazonov force-pushed the convertto-json-escapehandling branch from 3ff21c6 to 4cf4378 Compare September 17, 2018 11:24

SteveL-MSFT requested changes Sep 17, 2018

View reviewed changes

SteveL-MSFT requested changes Sep 19, 2018

View reviewed changes

Fix bug in test and use TestCases

7ef9f10

iSazonov force-pushed the convertto-json-escapehandling branch from 24ffde6 to 7ef9f10 Compare September 20, 2018 07:52

iSazonov requested review from BrucePay and daxian-dbw as code owners September 20, 2018 08:11

iSazonov force-pushed the convertto-json-escapehandling branch from 6df405a to 7ef9f10 Compare September 20, 2018 08:13

iSazonov closed this Sep 20, 2018

iSazonov reopened this Sep 20, 2018

iSazonov mentioned this pull request Sep 20, 2018

New EscapeHandling parameter in ConvertTo-Json cmdlet MicrosoftDocs/PowerShell-Docs#2941

Closed

8 tasks

SteveL-MSFT reviewed Sep 21, 2018

View reviewed changes

SteveL-MSFT requested changes Sep 24, 2018

View reviewed changes

Revert test change

0a6516a

SteveL-MSFT approved these changes Sep 25, 2018

View reviewed changes

TravisEz13 approved these changes Oct 1, 2018

View reviewed changes

markekraus approved these changes Oct 2, 2018

View reviewed changes

iSazonov merged commit d862d40 into PowerShell:master Oct 2, 2018

iSazonov deleted the convertto-json-escapehandling branch October 2, 2018 05:54

joeyaiello mentioned this pull request Oct 15, 2018

Docs needed for 'Add EscapeHandling parameter in ConvertTo-Json cmdlet' MicrosoftDocs/PowerShell-Docs#3023

Closed

joeyaiello removed Documentation Needed in this repo Documentation is needed in this repo labels Oct 15, 2018

Conversation

iSazonov commented Sep 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Summary

PR Checklist

Uh oh!

iSazonov commented Sep 13, 2018

Uh oh!

PlagueHO commented Sep 13, 2018

Uh oh!

iSazonov commented Sep 13, 2018

Uh oh!

TravisEz13 commented Sep 13, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iSazonov Sep 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iSazonov commented Sep 20, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iSazonov commented Sep 26, 2018

Uh oh!

markekraus left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

iSazonov commented Sep 13, 2018 •

edited

Loading

iSazonov Sep 18, 2018 •

edited

Loading