Make -NoTypeInformation Default on Export-Csv and ConvertTo-Csv #5164

markekraus · 2017-10-19T09:37:52Z

Sets -NoTypeInformation as the default behavior for Export-Csv and ConvertTo-Csv
Hides the -NoTypeInformation parameter switch
Adds -IncludeTypeInformation switch to Export-Csv and ConvertTo-Csv to enable legacy behavior
Provides a terminating error when both -NoTypeInformation and -IncludeTypeInformation are supplied
adds tests for the new behavior
fixes existing tests to align with new behavior

Breaking change was approved by committee in #5131

The new behavior will need to be documented.

iSazonov · 2017-10-19T14:13:40Z

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

+            }
+        }
+        private bool _includeTypeInformation;
+        private bool _includeTypeInformationIsSet;


In ProcessRecord() we use only if (NoTypeInformation == false).
So I believe we should use auto-implemented property for IncludeTypeInformation and NoTypeInformation and check IncludeTypeInformation.IsPresent.

I did testing with that.. and... it resulted in conflicts when using @splatting and supplying both. :( I ultimately went with this method because it was the most reliable way to detect if both were supplied.

edit: for clarity, when you supply $false to a switch parameter, IsPresent reports false. either by -Switch:$false or $hash = @{Switch = $false}; command @hash

As for @splatting - maybe it is a bug in @splatting ?
I think we should remove NoTypeInformation everywhere in code and leave only its declaration - the parameter is dummy and we should ignore it in code at all.

It's not a bug in splatting. Its the nature of how switch params work. IsPresent is does not mean was supplied but more user supplied $true

Function Test-SwitchParam { param( [Switch]$Switch ) "Switch: {0}" -f $Switch "Switch.IsPresent: {0}" -f $Switch.IsPresent " " } 'Test-SwitchParam' Test-SwitchParam 'Test-SwitchParam -Switch' Test-SwitchParam -Switch 'Test-SwitchParam -Switch:$true' Test-SwitchParam -Switch:$true 'Test-SwitchParam -Switch:$false' Test-SwitchParam -Switch:$false '$Params = @{Switch = $true}; Test-SwitchParam @Params' $Params = @{Switch = $true}; Test-SwitchParam @Params '$Params = @{Switch = $false}; Test-SwitchParam @Params' $Params = @{Switch = $false}; Test-SwitchParam @Params

Test-SwitchParam Switch: False Switch.IsPresent: False Test-SwitchParam -Switch Switch: True Switch.IsPresent: True Test-SwitchParam -Switch:$true Switch: True Switch.IsPresent: True Test-SwitchParam -Switch:$false Switch: False Switch.IsPresent: False $Params = @{Switch = $true}; Test-SwitchParam @Params Switch: True Switch.IsPresent: True $Params = @{Switch = $false}; Test-SwitchParam @Params Switch: False Switch.IsPresent: False

I think we should remove NoTypeInformation everywhere in code and leave only its declaration - the parameter is dummy and we should ignore it in code at all.

I'm torn on that. It's still an active parameter and setting it still has impact and will continue to do so. It's just hidden in favor of the new switch to make the new default behavior more clear. The internal logic would all need to be reworked. I didn't see value in switching all the logic to use IncludeTypeInformation when all that needs to be done is the upfront logic of detecting if both were supplied and setting NoTypeInformation based on the presence and setting of -IncludeTypeInformation. The logic would still need to be there, so why disturb the rest of the code for it?

for clarity: -NoTypeInformation:$false -NoTypeInformation:$true are both still valid.

I see - somebody can use -NoTypeInformation:$false.
However, I think we should make the code simpler and more readable. Currently -IncludeTypeInformation:$false don't work. I vote for removing many private variables and using IsPresent.

Currently -IncludeTypeInformation:$false don't work

It does currently work, that's not new functionality. The decision by the committee was to maintain that functionality. This is how we preserve that functionality while adding a new parameter that is basically a reverse switch for the existing one. You can't move to IsPresent without breaking current functionality.

@iSazonov I think I can make us both happy using this.MyInvocation.BoundParameters.ContainsKey(). I'll work on it.

After nice walking and good dinner I was ready to agree with you 😄

iSazonov · 2017-10-19T14:15:55Z

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

@@ -45,27 +45,44 @@ Describe "ConvertTo-Csv" -Tags "CI" {
    }

    It "Should output an array of objects" {
-        $result = $testObject | ConvertTo-Csv
+        $result = $testObject | ConvertTo-Csv -IncludeTypeInformation


I believe we shouldn't add -IncludeTypeInformation in tests where we don't check ""#TYPE ".

Ah, I miss understood what this one was testing. I will fix it.

iSazonov · 2017-10-19T14:18:11Z

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

+    It "Includes type information when -IncludeTypeInformation is supplied" {
+        $result = $testObject | ConvertTo-Csv -IncludeTypeInformation
+
+        ($result -split ([Environment]::NewLine))[0] | Should BeExactly "#TYPE System.Management.Automation.PSCustomObject"


I think we need add reverse test to check that ""#TYPE " is absent if -IncludeTypeInformation is absent.
Also that -NoTypeInformation do nothing.

sure I'll add those and do the same for the Export-Csv tests

iSazonov · 2017-10-19T17:02:26Z

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

@@ -100,6 +96,16 @@ public virtual void WriteCsvLine(string line)
        /// </summary>
        protected override void BeginProcessing()
        {
+            if (this.MyInvocation.BoundParameters.ContainsKey("IncludeTypeInformation") && this.MyInvocation.BoundParameters.ContainsKey("NoTypeInformation"))


Interesting, can we replace the string names with strong typed names?

What about different parameter sets?

can we replace the string names with strong typed names?

I'm not sure I understand what you are asking. IncludeTypeInformation.GetType().Name instead of "IncludeTypeInformation"?

What about different parameter sets?

parameter sets wont work without creating a bunch more. The parameter sets are currently being used for culture vs delimiter. It would get messy and harder to maintain than a logic check, IMO.

In the future, we need to revisit how to make defining and reading mutual exclusion of parameters easier. Using multiple parametersets today can be quite complex to get right. For now, having a runtime check is ok.

@SteveL-MSFT I created #5175 to track/discuss mutually exclusive parameters from a general position.

I meant something like nameof(param1).

@iSazonov thanks. fixed.

iSazonov · 2017-10-19T17:10:16Z

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

@@ -50,22 +50,47 @@ Describe "ConvertTo-Csv" -Tags "CI" {
    }

    It "Should return the type of data in the first element of the output array" {
-	$result = $testObject | ConvertTo-Csv
+	$result = $testObject | ConvertTo-Csv -IncludeTypeInformation

 	$result[0] | Should Be "#TYPE System.Management.Automation.PSCustomObject"
    }

    It "Should return the column info in the second element of the output array" {


We could remove the test - it is dup test in line 28.
Or it is better move the checks and next test to the test in line 28 - there we can check a type, a header and data.

It is minor comment.

@iSazonov Which test do you want to remove? the one you comment is on (lin 58) is not a duplicate of any of the tests. Line 28 is a delimiter test which needs to remain as its a different logic gate (parameter set).

the test on line 52 is now redundant with the the test on line 84, though.

It seems we could try to refactor the tests but not in the PR.
So skip the minor comment.

iSazonov · 2017-10-20T09:38:21Z

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

@@ -96,7 +96,7 @@ public virtual void WriteCsvLine(string line)
        /// </summary>
        protected override void BeginProcessing()
        {
-            if (this.MyInvocation.BoundParameters.ContainsKey("IncludeTypeInformation") && this.MyInvocation.BoundParameters.ContainsKey("NoTypeInformation"))
+            if (this.MyInvocation.BoundParameters.ContainsKey(nameof(IncludeTypeInformation)) && this.MyInvocation.BoundParameters.ContainsKey(nameof(NoTypeInformation)))


Can we use different parameter sets to exclude the code?

@iSazonov we already discussed that here

#5164 (comment)

The parameter sets are currently being used for delimiter vs culture. That would require we add 2 more parameter sets. It would make the code harder to maintain. The parameter sets would be confusing to the user because we are hiding -NoTypeInformation with DontShow=true. @SteveL-MSFT agreed that the runtime check was fine for now until we could address the larger general problem of mutex parameters. which prompted issue #5175

Sorry, I skipped this.

Make NoTypeInformation default on Export-Csv

2d5e6f7

PowerShell#5131 Demo

markekraus added Breaking-Change breaking change that may affect users Documentation Needed in this repo Documentation is needed in this repo labels Oct 19, 2017

markekraus requested review from dantraMSFT, JamesWTruher and PaulHigin as code owners October 19, 2017 09:37

[Feature] Run feature tests

6a5c134

markekraus mentioned this pull request Oct 19, 2017

-NoTypeInformation should be default on Export-Csv #5131

Closed

iSazonov reviewed Oct 19, 2017

View reviewed changes

iSazonov self-assigned this Oct 19, 2017

markekraus added 2 commits October 19, 2017 11:13

[Feature] Check for Parameter Binding

afbcd45

[Feature] Address PR feedback on tests

1fec825

iSazonov reviewed Oct 19, 2017

View reviewed changes

[Feature] use nameof() instead of string literals.

81e4e7f

iSazonov reviewed Oct 20, 2017

View reviewed changes

iSazonov approved these changes Oct 20, 2017

View reviewed changes

SteveL-MSFT approved these changes Oct 21, 2017

View reviewed changes

iSazonov merged commit 9fd6004 into PowerShell:master Oct 21, 2017

markekraus mentioned this pull request Oct 21, 2017

Document New default Behavior for Export-Csv and ConvertTo-Csv MicrosoftDocs/PowerShell-Docs#1795

Merged

5 tasks

markekraus removed the Documentation Needed in this repo Documentation is needed in this repo label Nov 19, 2017

markekraus deleted the ExportCsvNoTypeInformation branch January 19, 2018 18:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make -NoTypeInformation Default on Export-Csv and ConvertTo-Csv #5164

Make -NoTypeInformation Default on Export-Csv and ConvertTo-Csv #5164

markekraus commented Oct 19, 2017

iSazonov Oct 19, 2017

markekraus Oct 19, 2017 •

edited

iSazonov Oct 19, 2017

markekraus Oct 19, 2017 •

edited

iSazonov Oct 19, 2017

markekraus Oct 19, 2017

markekraus Oct 19, 2017

markekraus Oct 19, 2017

iSazonov Oct 19, 2017

iSazonov Oct 19, 2017

markekraus Oct 19, 2017

markekraus Oct 19, 2017

iSazonov Oct 19, 2017

markekraus Oct 19, 2017

markekraus Oct 19, 2017

iSazonov Oct 19, 2017

markekraus Oct 19, 2017

SteveL-MSFT Oct 19, 2017

markekraus Oct 19, 2017 •

edited

iSazonov Oct 20, 2017

markekraus Oct 20, 2017

iSazonov Oct 19, 2017

markekraus Oct 20, 2017

iSazonov Oct 20, 2017

iSazonov Oct 20, 2017

markekraus Oct 20, 2017

iSazonov Oct 20, 2017

Make -NoTypeInformation Default on Export-Csv and ConvertTo-Csv #5164

Make -NoTypeInformation Default on Export-Csv and ConvertTo-Csv #5164

Conversation

markekraus commented Oct 19, 2017

Choose a reason for hiding this comment

markekraus Oct 19, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markekraus Oct 19, 2017 • 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

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

markekraus Oct 19, 2017 • 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

markekraus Oct 19, 2017 •

edited

markekraus Oct 19, 2017 •

edited

markekraus Oct 19, 2017 •

edited