Add support W3C Extended Log File Format in Import-Csv #2482

iSazonov · 2016-10-14T15:48:57Z

Add support W3C Extended Log File Format
Add tests and new csv file for tests

msftclas · 2016-10-14T15:49:01Z

Hi @iSazonov, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!

The agreement was validated by Microsoft and real humans are currently evaluating your PR.

TTYL, MSBOT;

mirichmo · 2016-10-14T17:20:58Z

@adityapatwardhan and @JamesWTruher Please review

JamesWTruher · 2016-10-17T18:19:10Z

test/powershell/Modules/Microsoft.PowerShell.Utility/Import-Csv.Tests.ps1

+            }
+            It "Should be able to call without error" {
+                { Import-Csv $testCsv @HeaderParams } | Should Not Throw
+            }


I think this test can be removed, if the next test throws we get this test anyway

JamesWTruher · 2016-10-17T18:19:12Z

test/powershell/Modules/Microsoft.PowerShell.Utility/Import-Csv.Tests.ps1

+
+                $actual                     | Should Not BeNullOrEmpty
+                $actual.GetType().BaseType  | Should Be array
+                $actual.count               | Should Be 4


would it be useful to actually verify an imported value?

Test ("Should be able to import rights values") that checks the imported values is below the test.
I think this checks of return type and count of records should be.

JamesWTruher · 2016-10-17T18:20:30Z

test/powershell/Modules/Microsoft.PowerShell.Utility/Import-Csv.Tests.ps1

+                $actualAlias[0].'Column1'  | Should Be $actualCmdlet[0].'Column1'
+                $actualAlias[0].'Column2'  | Should Be $actualCmdlet[0].'Column2' 
+                $actualAlias[0].'Column 3' | Should Be $actualCmdlet[0].'Column 3'
+            }


yah - remove this test - just validate that the alias is present and is set correctly. There is no real need to actually invoke the alias

Both alias test are removed.

JamesWTruher · 2016-10-17T18:22:03Z

test/powershell/Modules/Microsoft.PowerShell.Utility/Import-Csv.Tests.ps1

+
+    AfterAll {
+        Remove-Item -Path $testfile -Force
+    }


since $TESTDRIVE is cleaned up automatically by Pester, this shouldn't be needed

JamesWTruher · 2016-10-17T18:22:52Z

I'm a little worried about backward compatibility here - I didn't see any discussion about that.

iSazonov · 2016-10-18T08:44:11Z

@JamesWTruher Thanks for review!
And I only see one breaking scenario when using "dirty" trick:

$temp = import-csv -path $testfile -header $customheader
# Skip first lines starting with '#'
$result = $temp | Select -Skip 5

I personally never use this technique.
I cannot analyze the powershell scripts on GitHub as @daxian-dbw AnalyzeSelectObject.ps1

Certainly there are many personal scripts that parse logs and that can be broken. But I think that their authors will be happy to see that now they can read this format logs directly without workarrounds.

iSazonov · 2016-10-25T12:34:39Z

Now I analyzed the powershell scripts on GitHub as @daxian-dbw (AnalyzeSelectObject.ps1)
From 1106 files I could find only one (ComputerManagement.psm1) that uses the above method but definitely not for a W3C log format.

mirichmo · 2016-11-07T19:19:26Z

test/powershell/Modules/Microsoft.PowerShell.Utility/assets/TestImportCsv2.csv

-data2,2,B
-data3,3,C
-data4,4,D
+Column1,Column2,Column 3


I'd like to see another test case similar to this one that contains a comment as the first line. It can be as simple as "# Comment". This case is covered by the existing Import-Csv cmdlet and it'd be nice to prove it still works as well.

mirichmo · 2016-11-07T19:21:12Z

test/powershell/Modules/Microsoft.PowerShell.Utility/Import-Csv.Tests.ps1

+                    # The file does not have header
+                    # (w/o Delimiter here we get throw (bug?)) 
+                    $HeaderParams = @{Header = $orginalHeader; Delimiter = ","}
+                    $CustomHeaderParams = @{Header = $customHeader; Delimiter = ","}


This line is duplicated and it is safe to move to line 41

mirichmo · 2016-11-07T19:23:21Z

test/powershell/Modules/Microsoft.PowerShell.Utility/Import-Csv.Tests.ps1

+            }
+
+            It "Should be able to assign to a variable" {
+                $actual = Import-Csv -Path $testCsv @HeaderParams


This test is redundant because all the other tests verify the same condition as well. If Import-Csv cannot assign the result to a variable, you won't be able to validate that it imported the correct headers and values

I think that the essence of the test is $actual.count | Should Be 4
No other test does not check this.

$actual.count | Should Be 4 would fit nicely on line 75. Then this test won't be needed.

mirichmo · 2016-11-07T19:28:09Z

test/powershell/Modules/Microsoft.PowerShell.Utility/Import-Csv.Tests.ps1

+                $actualfields | Should Be $customHeader
+            }
+
+            It "Should be able to import rights values" {


"right" or "correct"

mirichmo · 2016-11-07T19:37:19Z

test/powershell/Modules/Microsoft.PowerShell.Utility/assets/TestImportCsv1.csv

@@ -0,0 +1,4 @@
+data1,1,A


Descriptive file names would be nice to have here. Something like:
TestImportCsv1 --> TestImportCsv_NoHeader
TestImportCsv2 --> TestImportCsv_WithHeader
TestImportCsv3 --> TestImportCsv_W3C_ELF

1. Add support W3C Extended Log File Format 2. Refactoring Import-Csv tests 3. Add #Type tests

mirichmo · 2016-11-12T01:21:54Z

@iSazonov The change and tests look good based on my limited experience with the cmdlet. My only concern is the discussion earlier in the thread regarding this change's potential to be a "breaking change" of the existing behavior.
Can you please be more specific regarding the scenarios you investigated and their results?

iSazonov · 2016-11-12T10:35:20Z

@mirichmo Thanks for review!

I know only one breaking scenario. I described it above. This is exactly that I was looking for in scripts on GitHub for analysis. I have found no scripts with the breaking scenario.
So the question is whether there are other breaking scenarios?

Maybe we should ask Powershell team for help and then Powershell committee to make conclusion?

mirichmo · 2016-11-17T05:52:40Z

@SteveL-MSFT Should this be evaluated by the committee?

iSazonov · 2016-11-18T05:13:41Z

@mirichmo Thanks for the review!

SteveL-MSFT · 2016-11-18T16:51:49Z

@mirichmo since this is a breaking change, @PowerShell/powershell-committee will review this

iSazonov · 2016-11-21T05:17:05Z

@SteveL-MSFT I found another breaking scenario with custom file:

#, Field2, Field3
#1, Data11, Data12
#2, Data21, Data22

Workarround: explicitly use Header

msftclas added the cla-already-signed label Oct 14, 2016

daxian-dbw added the Review - Needed label Oct 14, 2016

iSazonov force-pushed the importcsv branch 2 times, most recently from 1115a94 to d6f25ce Compare October 17, 2016 11:34

JamesWTruher reviewed Oct 17, 2016

View reviewed changes

JamesWTruher self-assigned this Oct 17, 2016

lzybkr assigned mirichmo Nov 3, 2016

mirichmo suggested changes Nov 7, 2016

View reviewed changes

iSazonov added Review - Waiting on Author and removed Review - Needed labels Nov 7, 2016

iSazonov added 3 commits November 8, 2016 07:57

Add support W3C Extended Log File Format

89cce8f

1. Add support W3C Extended Log File Format 2. Refactoring Import-Csv tests 3. Add #Type tests

Fx test after review

404c56d

Fix tests after review

2515615

iSazonov force-pushed the importcsv branch from 78a0013 to 2515615 Compare November 8, 2016 05:20

Remove unneeded test

0818b6c

PowerShellTeam added Review - Needed and removed Review - Waiting on Author labels Nov 10, 2016

mirichmo approved these changes Nov 18, 2016

View reviewed changes

mirichmo merged commit 469924d into PowerShell:master Nov 18, 2016

iSazonov deleted the importcsv branch November 18, 2016 05:13

SteveL-MSFT added Review - Committee The PR/Issue needs a review from the PowerShell Committee Breaking-Change breaking change that may affect users labels Nov 18, 2016

SteveL-MSFT removed the Review - Committee The PR/Issue needs a review from the PowerShell Committee label Feb 2, 2017

iSazonov added the Documentation Needed in this repo Documentation is needed in this repo label Jun 17, 2017

joeyaiello mentioned this pull request Oct 15, 2018

Docs needed for 'Add support W3C Extended Log File Format in Import-Csv' MicrosoftDocs/PowerShell-Docs#3112

Closed

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support W3C Extended Log File Format in Import-Csv #2482

Add support W3C Extended Log File Format in Import-Csv #2482

iSazonov commented Oct 14, 2016

msftclas commented Oct 14, 2016

mirichmo commented Oct 14, 2016

JamesWTruher Oct 17, 2016

iSazonov Oct 18, 2016

JamesWTruher Oct 17, 2016

iSazonov Oct 18, 2016

JamesWTruher Oct 17, 2016

iSazonov Oct 18, 2016

JamesWTruher Oct 17, 2016

iSazonov Oct 18, 2016

JamesWTruher commented Oct 17, 2016

iSazonov commented Oct 18, 2016

iSazonov commented Oct 25, 2016

mirichmo Nov 7, 2016

iSazonov Nov 8, 2016 •

edited

mirichmo Nov 7, 2016

iSazonov Nov 8, 2016

mirichmo Nov 7, 2016

iSazonov Nov 8, 2016

mirichmo Nov 8, 2016

iSazonov Nov 9, 2016

mirichmo Nov 7, 2016

iSazonov Nov 8, 2016

mirichmo Nov 7, 2016

iSazonov Nov 8, 2016

mirichmo commented Nov 12, 2016

iSazonov commented Nov 12, 2016

mirichmo commented Nov 17, 2016

iSazonov commented Nov 18, 2016

SteveL-MSFT commented Nov 18, 2016

iSazonov commented Nov 21, 2016

Add support W3C Extended Log File Format in Import-Csv #2482

Add support W3C Extended Log File Format in Import-Csv #2482

Conversation

iSazonov commented Oct 14, 2016

msftclas commented Oct 14, 2016

mirichmo commented Oct 14, 2016

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

JamesWTruher commented Oct 17, 2016

iSazonov commented Oct 18, 2016

iSazonov commented Oct 25, 2016

Choose a reason for hiding this comment

iSazonov Nov 8, 2016 • edited

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

Choose a reason for hiding this comment

mirichmo commented Nov 12, 2016

iSazonov commented Nov 12, 2016

mirichmo commented Nov 17, 2016

iSazonov commented Nov 18, 2016

SteveL-MSFT commented Nov 18, 2016

iSazonov commented Nov 21, 2016

iSazonov Nov 8, 2016 •

edited