Skip to content

Conversation

@Topping
Copy link
Contributor

@Topping Topping commented Oct 1, 2018

PR Summary

Import-Csv no longer skips double quotation marks, when using " as delimiter, as referenced in #7110

Added tests for possible regressions.

PR Checklist

@msftclas
Copy link

msftclas commented Oct 1, 2018

CLA assistant check
All CLA requirements met.

@Topping Topping closed this Oct 1, 2018
@Topping Topping reopened this Oct 1, 2018
@iSazonov iSazonov requested a review from SteveL-MSFT October 1, 2018 10:02
@iSazonov iSazonov self-assigned this Oct 1, 2018
Renamed to clarify purpose of testing with double quotes
@iSazonov iSazonov changed the title Double quote delimiter import csv issue 7110 No longer skips double quote delimiter in Import-Csv Oct 2, 2018
@iSazonov iSazonov added the Hacktoberfest Potential candidate to participate in Hacktoberfest label Oct 2, 2018
@@ -0,0 +1,2 @@
a1""a3
Copy link
Member

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

Copy link
Contributor Author

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 = '"' }
Copy link
Member

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

Copy link
Contributor Author

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.
@{ 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
Copy link
Collaborator

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.

Copy link
Member

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.

@iSazonov iSazonov merged commit 20b2e3d into PowerShell:master Oct 5, 2018
@iSazonov iSazonov changed the title No longer skips double quote delimiter in Import-Csv No longer skips a column without name if double quote delimiter is used in Import-Csv Oct 5, 2018
@iSazonov
Copy link
Collaborator

iSazonov commented Oct 5, 2018

@Topping Thanks for your contribution!

adityapatwardhan pushed a commit to adityapatwardhan/PowerShell that referenced this pull request Apr 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Hacktoberfest Potential candidate to participate in Hacktoberfest

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants