Remove a type conversion for CommaDelimitedStringCollection #11000
Conversation
| if (toType == typeof(StringCollection)) | ||
| { | ||
| ConversionRank rank = (fromType.IsArray || IsTypeEnumerable(fromType)) ? ConversionRank.Language : ConversionRank.LanguageS2A; | ||
| return CacheConversion<StringCollection>(fromType, toType, LanguagePrimitives.ConvertToStringCollection, rank); |
There was a problem hiding this comment.
@iSazonov, your last commit had 1 failures in PowerShell-CI-macos
Validate start of console host.No new assemblies are loaded
Expected exactly $null, but got @{InputObject=System.Configuration.ConfigurationManager.dll; SideIndicator==>}.
at <ScriptBlock>, /Users/runner/runners/2.160.0/work/1/s/test/powershell/Host/Startup.Tests.ps1: line 125
125: $diffs | Should -BeExactly $null| if (toType == typeof(StringCollection)) | ||
| { | ||
| ConversionRank rank = (fromType.IsArray || IsTypeEnumerable(fromType)) ? ConversionRank.Language : ConversionRank.LanguageS2A; | ||
| return CacheConversion<StringCollection>(fromType, toType, LanguagePrimitives.ConvertToStringCollection, rank); |
There was a problem hiding this comment.
@iSazonov, your last commit had 2 failures in PowerShell-CI-windows
Enter-PSHostProcess tests.By CustomPipeName.Can enter, exit, and re-enter using CustomPipeName
Expected $true, because The script was able to re-enter another process and grab the pipe of 'lste30rz.0ra'., but got $false.
at <ScriptBlock>, D:\a\1\s\test\powershell\Modules\Microsoft.PowerShell.Core\Enter-PSHostProcess.Tests.ps1: line 170
170: Should -BeTrue -Because "The script was able to re-enter another process and grab the pipe of '$pipeName'."Validate start of console host.No new assemblies are loaded
Expected exactly $null, but got @{InputObject=System.Configuration.ConfigurationManager.dll; SideIndicator==>}.
at <ScriptBlock>, D:\a\1\s\test\powershell\Host\Startup.Tests.ps1: line 125
125: $diffs | Should -BeExactly $null| if (toType == typeof(StringCollection)) | ||
| { | ||
| ConversionRank rank = (fromType.IsArray || IsTypeEnumerable(fromType)) ? ConversionRank.Language : ConversionRank.LanguageS2A; | ||
| return CacheConversion<StringCollection>(fromType, toType, LanguagePrimitives.ConvertToStringCollection, rank); |
There was a problem hiding this comment.
@iSazonov, your last commit had 1 failures in PowerShell-CI-linux
Validate start of console host.No new assemblies are loaded
Expected exactly $null, but got @{InputObject=System.Configuration.ConfigurationManager.dll; SideIndicator==>}.
at <ScriptBlock>, /home/vsts/work/1/s/test/powershell/Host/Startup.Tests.ps1: line 125
125: $diffs | Should -BeExactly $null| 'System.ComponentModel.dll' | ||
| 'System.ComponentModel.Primitives.dll' | ||
| 'System.ComponentModel.TypeConverter.dll' | ||
| 'System.Configuration.ConfigurationManager.dll' |
There was a problem hiding this comment.
It's not worth it to add this converter to the cache at the very beginning.
This dll is 959 kb, almost 1mb, and this converter will rarely to be used (only for compatibility purpose). Pulling in the dll at startup time is not right.
There was a problem hiding this comment.
Yes :-). Have you an idea how add ConvertToCommaDelimitedStringCollection() not at startup?
- I wonder why did Windows PowerShell add this in cache?
- Also I wonder why do we load so many dll-s at startup although it seems they are not used.
There was a problem hiding this comment.
We could add new converter using TypeTable_Types_Ps1Xml.cs like Microsoft.PowerShell.DeserializingTypeConverter but I guess that typeof(CommaDelimitedStringCollection) reference in the converter will load the assembly at startup. So we need a conclusion do we want to return back the Windows PowerShell feature.
Update: On Github I don't find PowerShell scripts using CommaDelimitedStringCollection. So we could remove the code as formally breaking change.
There was a problem hiding this comment.
@daxian-dbw @SteveL-MSFT Could you please make a conclusion should we add back the converter?
.Net Core (and .Net Framework) does not use it on CommaDelimitedStringCollection by default. I don't know why it was added in Windows PowerShell.
I think we could remove the code.
There was a problem hiding this comment.
Since PSCore6 didn't have this and I haven't seen any complaints about it along with your GitHub search it seems unlikely anyone missed that this was gone. So it seem we should not add it back given the size.
There was a problem hiding this comment.
Great, then this PR can be closed 😄
There was a problem hiding this comment.
I removed the old code in the PR and updated the PR description.
2de5f67 to
3740994
Compare
3740994 to
cd46f2d
Compare
|
🎉 Handy links: |
PR Summary
Now CommaDelimitedStringCollection class is available in .Net Core 3.0 but after a discussion we decided to remove the old code as very rarely used.
Users can add the converter with Update-TypeData cmdlet if needed.
PR Context
PR Checklist
.h,.cpp,.cs,.ps1and.psm1files have the correct copyright headerWIP:or[ WIP ]to the beginning of the title (theWIPbot will keep its status check atPendingwhile the prefix is present) and remove the prefix when the PR is ready.