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

Charlie Schmidt (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

Aditya Patwardhan (adityapatwardhan) · 2018-01-08T22:39:54Z

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


Please spell out implementation. impl -> implementation

Aditya Patwardhan (adityapatwardhan) · 2018-01-08T22:40:58Z

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


Please spell out implementation. impl -> implementation

Aditya Patwardhan (adityapatwardhan) · 2018-01-08T22:41:48Z


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


Please spell out implementation. imple -> implementation

Aditya Patwardhan (adityapatwardhan) · 2018-01-08T22:48:48Z

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


Please use named variables.

Aditya Patwardhan (adityapatwardhan) · 2018-01-08T22:49:47Z

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


Consider rewording, BegingProcessing override to create the XML scaffolding.

Aditya Patwardhan (adityapatwardhan) · 2018-01-09T00:22:24Z

+                }
+            }
+
+            $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.

Aditya Patwardhan (adityapatwardhan) · 2018-01-09T00:22:37Z

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


Extra lines

Aditya Patwardhan (adityapatwardhan) · 2018-01-09T00:23:33Z

+                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

Aditya Patwardhan (adityapatwardhan) · 2018-01-09T00:24:00Z

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


Use Should Not BeNullOrEmpty instead

Aditya Patwardhan (adityapatwardhan) · 2018-01-09T00:24:39Z

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


Same as above.

Charlie Schmidt (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

Aditya Patwardhan (adityapatwardhan) · 2018-01-11T17:52:13Z

+            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 {

Aditya Patwardhan (adityapatwardhan) · 2018-01-11T17:52:45Z

+            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

Aditya Patwardhan (adityapatwardhan) · 2018-01-11T17:53:38Z

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


Use named parameter.

Aditya Patwardhan (adityapatwardhan) · 2018-01-11T17:54:50Z

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


Extra line.

Aditya Patwardhan (adityapatwardhan) · 2018-01-11T17:55:18Z

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


Add comment here.

Aditya Patwardhan (adityapatwardhan) · 2018-01-11T18:10:00Z

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


Extra line.

Aditya Patwardhan (adityapatwardhan) · 2018-01-11T18:21:27Z

Charlie Schmidt (@charlieschmidt) Thanks for adding the documentation, much appreciated.

Charlie Schmidt (charlieschmidt) · 2018-01-11T19:21:08Z

Aditya Patwardhan (@adityapatwardhan) yup. Anything else needed?

…From-Clixml test cases

Ilya (iSazonov)

Charlie Schmidt (@charlieschmidt) Thanks for your contribution!

Ilya (iSazonov) · 2018-01-12T04:31:08Z


            $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.

Ilya (iSazonov) · 2018-01-12T04:31:23Z


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


It seems it is unrelated the PR - please revert.

Ilya (iSazonov) · 2018-01-12T04:33:30Z

+            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".

Ilya (iSazonov) · 2018-01-12T04:34:06Z

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


The same - "serializes".

Ilya (iSazonov) · 2018-01-12T04:39:42Z


        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

Ilya (iSazonov) · 2018-01-12T04:43:58Z

+        $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.

Ilya (iSazonov) · 2018-01-15T03:06:35Z

 {
    /// <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.

Ilya (iSazonov) · 2018-01-15T03:13:18Z

+        #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.

Ilya (iSazonov) · 2018-01-15T03:25:09Z

+            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?

Aditya Patwardhan (adityapatwardhan) · 2018-01-18T00:46:31Z

Charlie Schmidt (@charlieschmidt) Can you respond to some comments from Ilya (@iSazonov)

Charlie Schmidt (charlieschmidt) · 2018-01-18T02:55:11Z

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

Aditya Patwardhan (adityapatwardhan) · 2018-01-18T18:05:51Z

Charlie Schmidt (@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.

Charlie Schmidt (charlieschmidt) · 2018-01-18T20:57:57Z

Yes please

Aditya Patwardhan (adityapatwardhan) · 2018-02-20T18:08:06Z

Charlie Schmidt (@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.

Charlie Schmidt (charlieschmidt) requested review from James Truher (JamesWTruher), Paul Higinbotham (PaulHigin) and Dan Travison (dantraMSFT) as code owners January 8, 2018 05:30

Dongbo Wang (daxian-dbw) assigned Aditya Patwardhan (adityapatwardhan) Jan 8, 2018

Aditya Patwardhan (adityapatwardhan) added the Documentation Needed in this repo Documentation is needed in this repo label Jan 8, 2018

Aditya Patwardhan (adityapatwardhan) requested changes Jan 9, 2018

View reviewed changes

Charlie Schmidt (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

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

Aditya Patwardhan (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

Ilya (iSazonov) reviewed Jan 12, 2018

View reviewed changes

Ilya (iSazonov) reviewed Jan 15, 2018

View reviewed changes

Aditya Patwardhan (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

Charlie Schmidt (charlieschmidt) closed this Mar 26, 2018

Charlie Schmidt (charlieschmidt) reopened this Mar 26, 2018

Charlie Schmidt (charlieschmidt) closed this Mar 26, 2018

stale Bot removed the Stale label Mar 26, 2018

Sean Wheeler (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

Joey Aiello (joeyaiello) mentioned this pull request Oct 15, 2018

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

Closed

Joey Aiello (joeyaiello) removed Documentation Needed in this repo Documentation is needed in this repo labels Oct 15, 2018

Michael Klement (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

Charlie Schmidt (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!

Charlie Schmidt (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!

Aditya Patwardhan (adityapatwardhan) commented Jan 11, 2018

Uh oh!

Charlie Schmidt (charlieschmidt) commented Jan 11, 2018

Uh oh!

Ilya (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!

Ilya (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!

Charlie Schmidt (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!

Ilya (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

Charlie Schmidt (charlieschmidt) commented Jan 8, 2018 •

edited

Loading

Ilya (iSazonov) Jan 12, 2018 •

edited

Loading

Charlie Schmidt (charlieschmidt) Jan 12, 2018 •

edited

Loading

Ilya (iSazonov) Jan 12, 2018 •

edited

Loading