-
Notifications
You must be signed in to change notification settings - Fork 8.1k
No longer skips a column without name if double quote delimiter is used in Import-Csv #7899
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
No longer skips a column without name if double quote delimiter is used in Import-Csv #7899
Conversation
...shell/Modules/Microsoft.PowerShell.Utility/assets/TestImportCsvQuoteDelimiter_EmptyValue.csv
Outdated
Show resolved
Hide resolved
test/powershell/Modules/Microsoft.PowerShell.Utility/Import-Csv.Tests.ps1
Outdated
Show resolved
Hide resolved
test/powershell/Modules/Microsoft.PowerShell.Utility/Import-Csv.Tests.ps1
Outdated
Show resolved
Hide resolved
Renamed to clarify purpose of testing with double quotes
| @@ -0,0 +1,2 @@ | |||
| a1""a3 | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would prefer to generate these at runtime than have it checked in as it's easier to see the test file in the test code than open another file. Something like:
$emptyValueCsv = @'
a1""a3
v1"v2"v3
'@
Set-Content testdrive:/EmptyValue.csv -Value $emptyValueCsvThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I looked into using a here-string earlier, but the lack of indentation for the string terminator made the code look quite messy.
I couldn't quite find an elegant way to add the here-string, but I've gone an added one for the different file contents.
| } | ||
|
|
||
| It "Should handle <name>" -TestCases @( | ||
| @{ name = "quote with empty value"; expectedHeader = "a1,H1,a3"; file = $TestImportCsvQuoteDelimiter_EmptyValue; delimiter = '"' } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This formatting seems off, lines 33-38 have one extra indentation. Also, it's nice to align the semicolons in the hashtable per row to make them easier to read
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Turns out those lines had tabs as indentation, instead of spaces like the rest of the lines.
Content for double quote delimiter test moved inside the test file.
Indented test file content for double quote tests
Few lines in import-csv.tests had tabs for indentation instead of spaces.
test/powershell/Modules/Microsoft.PowerShell.Utility/Import-Csv.Tests.ps1
Outdated
Show resolved
Hide resolved
| @{ name = "value enclosed in quote" ; expectedHeader = "a1,a2,a3"; file = "QuotedCharacter.csv" ; content = $quotedCharacterCsv ; delimiter = ',' } | ||
| ){ | ||
| param($expectedHeader, $file, $content, $delimiter) | ||
| Set-Content testdrive:/$file -Value $content |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Usualy we do this in BeforeAll block. Then we could pass only file name to the block.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having it in BeforeAll makes sense if multiple tests use the same test content, but in this case only this test uses them so I think it's ok to just have it here closer to the test code.
|
@Topping Thanks for your contribution! |
PR Summary
Import-Csv no longer skips double quotation marks, when using
"as delimiter, as referenced in #7110Added tests for possible regressions.
PR Checklist
.h,.cpp,.cs,.ps1and.psm1files have the correct copyright headerWIP:to the beginning of the title and remove the prefix when the PR is ready.[feature]if the change is significant or affects feature tests