WIP: Add ConvertTo-Clixml and ConvertFrom-Clixml cmdlets by charlieschmidt · Pull Request #5815 · PowerShell/PowerShell

charlieschmidt · 2018-01-08T05:30:15Z

PR Summary

ConvertTo-Clixml and ConvertFrom-Clixml cmdlets (to go with Export/Import-Clixml; like Import/Export/ConvertFrom/ConvertTo-Csv)

Refactored Export-Clixml into base functionality shared with ConvertTo-Clixml.

Import/ConvertFrom-Clixml both use existing ImportXmlHelper class - extended that to create MemoryStream for ConvertFrom-

Corresponding pester tests.

Closes #3898

PR Checklist

Note: Please mark anything not applicable to this PR NA.

PR has a meaningful title
- Use the present tense and imperative mood when describing your changes
Summarized changes
User facing Documentation needed
- Issue filed - Issue link: Cmdlet help for ConvertFrom-Clixml and ConvertTo-Clixml MicrosoftDocs/PowerShell-Docs#2022
Change is not breaking
Make sure you've added a new test if existing tests do not effectively test the code changed
- Add [feature] if the change is significant or affectes feature tests
This PR is ready to merge and is not Work in Progress.
- If the PR is work in progress, please add the prefix WIP: to the beginning of the title and remove the prefix when the PR is ready.

…port-Clixml; like Import/Export/ConvertFrom/ConvertTo-Csv) Refactored Export-Clixml into base functionality shared with ConvertTo-Clixml. Import/ConvertFrom-Clixml both use existing ImportXmlHelper class - extended that to create MemoryStream for ConvertFrom- Corresponding pester tests. Closes PowerShell#3898

adityapatwardhan · 2018-01-08T22:39:54Z

src/Microsoft.PowerShell.Commands.Utility/commands/utility/XmlCommands.cs

 {
    /// <summary>
-    /// implementation for the Export-Clixml command
+    /// base impl for ConvertTo-Clixml and Export-Clixml


Please spell out implementation. impl -> implementation

adityapatwardhan · 2018-01-08T22:40:58Z

src/Microsoft.PowerShell.Commands.Utility/commands/utility/XmlCommands.cs

-        #region Overrides
+        #region to-clixml 
+        /// <summary>
+        /// impl specific TextWriter for the xml serializer to use


Please spell out implementation. impl -> implementation

adityapatwardhan · 2018-01-08T22:41:48Z

src/Microsoft.PowerShell.Commands.Utility/commands/utility/XmlCommands.cs


        /// <summary>
-        /// BeginProcessing override
+        /// IDisposable style cleanup for any imple specific stuff, be sure to call base.CleanUp()


Please spell out implementation. imple -> implementation

adityapatwardhan · 2018-01-08T22:48:48Z

src/Microsoft.PowerShell.Commands.Utility/commands/utility/XmlCommands.cs

+            }
+            else
+            {
+                _serializer = new Serializer(_xw, Depth, true);


Please use named variables.

adityapatwardhan · 2018-01-08T22:49:47Z

src/Microsoft.PowerShell.Commands.Utility/commands/utility/XmlCommands.cs

+        #region PSCmdlet Members
+
+        /// <summary>
+        /// BeginProcessing override, create the xml scaffolding stuff


Consider rewording, BegingProcessing override to create the XML scaffolding.

adityapatwardhan · 2018-01-09T00:22:24Z

test/powershell/Modules/Microsoft.PowerShell.Utility/clixml.tests.ps1

+                }
+            }
+
+            $isExisted | Should Be $true


Typically the error message on console when a failure happens with assertions like this are not helpful.
Was false when true was expected.

adityapatwardhan · 2018-01-09T00:22:37Z

test/powershell/Modules/Microsoft.PowerShell.Utility/clixml.tests.ps1

+            $isExisted | Should Be $true
+        }
+
+


Extra lines

adityapatwardhan · 2018-01-09T00:23:33Z

test/powershell/Modules/Microsoft.PowerShell.Utility/clixml.tests.ps1

+                foreach($gpsItem in $gpsList)
+                {
+                    $checkId = $gpsItem.Id
+                    if (($null -ne $(Select-String -InputObject $item -SimpleMatch $checkId)) -and ($null -ne $(Select-String -InputObject $item -SimpleMatch "Id")))


Same comment as above with looping and select-string

adityapatwardhan · 2018-01-09T00:24:00Z

test/powershell/Modules/Microsoft.PowerShell.Utility/clixml.tests.ps1

+            $filePath | Should Exist
+
+            $fileContent = Get-Content $filePath -raw
+            $fileContent | Should Not Be $null


Use Should Not BeNullOrEmpty instead

adityapatwardhan · 2018-01-09T00:24:39Z

test/powershell/Modules/Microsoft.PowerShell.Utility/clixml.tests.ps1

+            $filePath | Should Exist
+
+            $fileContent = Get-Content $filePath
+            $fileContent | Should Not Be $null


Same as above.

charlieschmidt · 2018-01-09T05:29:43Z

Should address the review issues, and a few other spelling/correction stuff.

I've also added documentation and a PR for PowerShell-Docs at MicrosoftDocs/PowerShell-Docs#2022

Change to auto properties where appropriate Don't allow empty string as input to ConvertFrom-Clixml Fix IDisposable implementation in ConvertFrom-Clixml SImplify logic of ConvertFrom-Clixml Simplier/corrected tests

adityapatwardhan · 2018-01-11T17:52:13Z

test/powershell/Modules/Microsoft.PowerShell.Utility/clixml.tests.ps1

+            Remove-Item $filePath -Force -ErrorAction SilentlyContinue
+        }
+
+        It "deserializes an object as expected" {


These two tests seem very similar. Can we use -TestCases? Similar to

PowerShell/test/powershell/Language/Scripting/Requires.Tests.ps1

Line 14 in 02b5f35

It "throws ParserException - <testname>" -TestCases $testcases {

adityapatwardhan · 2018-01-11T17:52:45Z

test/powershell/Modules/Microsoft.PowerShell.Utility/clixml.tests.ps1

+            Remove-Item $filePath -Force -ErrorAction SilentlyContinue
+        }
+
+        It "converts an object to string as expected" {


These two tests seem very similar. Can we use -TestCases? Similar to

PowerShell/test/powershell/Language/Scripting/Requires.Tests.ps1

Line 14 in 02b5f35

It "throws ParserException - <testname>" -TestCases $testcases {

I don't think so, not without making it uglier. Because the difference in the two tests isn't a variable or parameter, it's function param vs pipeline. That said, can move some of the duplicate code to the BeforeAll, should make it more clear what is really being tested

adityapatwardhan · 2018-01-11T17:53:38Z

src/Microsoft.PowerShell.Commands.Utility/commands/utility/XmlCommands.cs

+            }
+            else
+            {
+                _serializer = new Serializer(_xmlWriter, Depth, true);


Use named parameter.

adityapatwardhan · 2018-01-11T17:54:50Z

src/Microsoft.PowerShell.Commands.Utility/commands/utility/XmlCommands.cs

        {
-            if (_disposed == false)
+            _stringWriter = new StringWriter();
+


Extra line.

adityapatwardhan · 2018-01-11T17:55:18Z

src/Microsoft.PowerShell.Commands.Utility/commands/utility/XmlCommands.cs

+        }
+
+        /// <summary>
+        ///


Add comment here.

adityapatwardhan · 2018-01-11T18:10:00Z

src/Microsoft.PowerShell.Commands.Utility/commands/utility/XmlCommands.cs

+            _inputObjectBuffer.Add(InputObject);
+        }
+
+


Extra line.

adityapatwardhan · 2018-01-11T18:21:27Z

@charlieschmidt Thanks for adding the documentation, much appreciated.

charlieschmidt · 2018-01-11T19:21:08Z

@adityapatwardhan yup. Anything else needed?

…From-Clixml test cases

iSazonov

@charlieschmidt Thanks for your contribution!

iSazonov · 2018-01-12T04:31:08Z

test/powershell/Modules/Microsoft.PowerShell.Utility/clixml.tests.ps1


            $fileContent = Get-Content $filePath
-            $fileContent | Should Not Be $null
+            $fileContent | Should Not BeNullOrEmpty


It seems it is unrelated the PR - please revert.

can, but was cleaning up other instances of this, want me to pull it to a separate PR or?

#5815 (comment)

Such changes increase the reviewing time because a reviewer must understand that this change is not related to code changes.

Thanks for clarify. I don't see the request. I am fine to leave the change.

Closed.

iSazonov · 2018-01-12T04:31:23Z

test/powershell/Modules/Microsoft.PowerShell.Utility/clixml.tests.ps1


            $fileContent = Get-Content $filePath
-            $fileContent | Should Not Be $null
+            $fileContent | Should Not BeNullOrEmpty


It seems it is unrelated the PR - please revert.

iSazonov · 2018-01-12T04:33:30Z

test/powershell/Modules/Microsoft.PowerShell.Utility/clixml.tests.ps1

+            Remove-Item $filePath -Force -ErrorAction SilentlyContinue
+        }
+
+        It "converts an object to string as expected" {


Below we use "deserializes" - so I believe here we should use "serializes".

iSazonov · 2018-01-12T04:34:06Z

test/powershell/Modules/Microsoft.PowerShell.Utility/clixml.tests.ps1

+            $objClixml | Should Match "<I32 N=`"Id`">$($gps.Id)</I32>"
+        }
+
+        It "converts an object from the pipeline to string as expected" {


The same - "serializes".

iSazonov · 2018-01-12T04:39:42Z

test/powershell/Modules/Microsoft.PowerShell.Utility/clixml.tests.ps1


        Import-Clixml -Path (Join-Path $PSScriptRoot "assets\CorruptedCim.clixml")

+        # Wait up to 10 seconds for calc.exe to run


Please use our Wait-UntilTrue function.

Thats a new function to me, will definitely add that

On second reading, may not be appropriate. WIth other uses of Wait-UntilTrue, we're waiting until something is true, with a 10second time out, checking every second. Here - we want to wait at least 10 seconds, checking to make sure that calc hasn't started

Maybe just

start-sleep -Seconds 10 Get-Process ... | Should BeNullOrEmpty

? I'm not as familiar with how a corrupted Cim class could kick off the exe (and have a mac anyway [and the whole thing may not apply in .netcore anyway who knows?]) - I just used the existing Import-Clixml test

You can put Get-Process Calc to a script block and send it to Wait-UntilTrue.

It seems Microsoft.Management.Infrastructure is still not ported #4562

iSazonov · 2018-01-12T04:43:58Z

test/powershell/Modules/Microsoft.PowerShell.Utility/clixml.tests.ps1

+        $cliXmlContent = Get-Content -Path (Join-Path $PSScriptRoot "assets\CorruptedCim.clixml") -Raw
+        ConvertFrom-CliXml -InputObject $cliXmlContent
+
        # Wait up to 10 seconds for calc.exe to run


It is not your code but please use our Wait-UntilTrue function too.

iSazonov · 2018-01-15T03:06:35Z

src/Microsoft.PowerShell.Commands.Utility/commands/utility/XmlCommands.cs

 {
    /// <summary>
-    /// implementation for the Export-Clixml command
+    /// Base implementation for ConvertTo-Clixml and Export-Clixml


The comment is public - please add final dot.
Below please review formatting public comments too.

iSazonov · 2018-01-15T03:13:18Z

src/Microsoft.PowerShell.Commands.Utility/commands/utility/XmlCommands.cs

+        #region FileStream implementation for ToClixmlCommand
+
+        /// <summary>
+        /// handle to file stream


Please remove the obvious comment . And why the comment is public for private field? Below too.

iSazonov · 2018-01-15T03:25:09Z

src/Microsoft.PowerShell.Commands.Utility/commands/utility/XmlCommands.cs

+            if (!successfullyConverted && _inputObjectBuffer.Count > 0)
+            {
+                _helper = new ImportXmlHelper(this, ImportXmlHelper.CreateMemoryStream(string.Join(System.Environment.NewLine, _inputObjectBuffer)));
+                _helper.Import();


We already create _helper - can we reuse it?

Can XmlException be raised here? If so do we catch it and throw use freadly exception?

adityapatwardhan · 2018-01-18T00:46:31Z

@charlieschmidt Can you respond to some comments from @iSazonov

charlieschmidt · 2018-01-18T02:55:11Z

Ponging to say am still following this and will respond with comments or commits tomorrow evening

adityapatwardhan · 2018-01-18T18:05:51Z

@charlieschmidt Please have a look at failing tests. Shall I go ahead and mark this PR as WIP: since there is more work expected here.

charlieschmidt · 2018-01-18T20:57:57Z

Yes please

adityapatwardhan · 2018-02-20T18:08:06Z

@charlieschmidt Please resolve merge conflict as well. Thanks!

stale · 2018-03-22T18:49:30Z

This PR has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed if no further activity occurs within 10 days. Thank you for your contributions.

charlieschmidt requested review from JamesWTruher, PaulHigin and dantraMSFT as code owners January 8, 2018 05:30

daxian-dbw assigned adityapatwardhan Jan 8, 2018

adityapatwardhan added the Documentation Needed in this repo Documentation is needed in this repo label Jan 8, 2018

adityapatwardhan requested changes Jan 9, 2018

View reviewed changes

charlieschmidt mentioned this pull request Jan 9, 2018

Cmdlet help for ConvertFrom-Clixml and ConvertTo-Clixml MicrosoftDocs/PowerShell-Docs#2022

Merged

8 tasks

Fix comments, spelling

9e26e79

Change to auto properties where appropriate Don't allow empty string as input to ConvertFrom-Clixml Fix IDisposable implementation in ConvertFrom-Clixml SImplify logic of ConvertFrom-Clixml Simplier/corrected tests

charlieschmidt force-pushed the powershell-3898-convertfromto-clixml branch from 3ed2b92 to 9e26e79 Compare January 9, 2018 05:51

adityapatwardhan requested changes Jan 11, 2018

View reviewed changes

Use named parameter for boolean; remove empty lines; clean up Convert…

79de6b3

…From-Clixml test cases

iSazonov reviewed Jan 12, 2018

View reviewed changes

iSazonov reviewed Jan 15, 2018

View reviewed changes

adityapatwardhan changed the title ~~Add ConvertTo-Clixml and ConvertFrom-Clixml cmdlets~~ WIP: Add ConvertTo-Clixml and ConvertFrom-Clixml cmdlets Jan 18, 2018

stale bot added the Stale label Mar 22, 2018

charlieschmidt closed this Mar 26, 2018

charlieschmidt reopened this Mar 26, 2018

charlieschmidt closed this Mar 26, 2018

stale bot removed the Stale label Mar 26, 2018

sdwheeler mentioned this pull request Jun 8, 2018

ConvertTo/From-CliXML cmdlet help added to v6 but cmdlets don't exist MicrosoftDocs/PowerShell-Docs#2533

Closed

8 tasks

joeyaiello mentioned this pull request Oct 15, 2018

Docs needed for 'WIP: Add ConvertTo-Clixml and ConvertFrom-Clixml cmdlets' MicrosoftDocs/PowerShell-Docs#3055

Closed

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

mklement0 mentioned this pull request Nov 25, 2018

Allow ConvertTo-Xml to emit CLIXML (the same XML format as Export-Clixml) #8311

Closed


		Import-Clixml -Path (Join-Path $PSScriptRoot "assets\CorruptedCim.clixml")

		# Wait up to 10 seconds for calc.exe to run

Conversation

charlieschmidt commented Jan 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Summary

PR Checklist

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!

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!

charlieschmidt commented Jan 9, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adityapatwardhan commented Jan 11, 2018

Uh oh!

charlieschmidt commented Jan 11, 2018

Uh oh!

iSazonov left a comment

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 Jan 12, 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!

charlieschmidt Jan 12, 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!

iSazonov Jan 12, 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

charlieschmidt commented Jan 8, 2018 •

edited

Loading

iSazonov Jan 12, 2018 •

edited

Loading

charlieschmidt Jan 12, 2018 •

edited

Loading

iSazonov Jan 12, 2018 •

edited

Loading