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

Unify cmdlets with parameter 'Encoding' to be of type System.Text.Encoding #5080

Merged
merged 8 commits into from Oct 24, 2017

Conversation

@JamesWTruher
Member

JamesWTruher commented Oct 10, 2017

This unifies file encoding across the inbox cmdlets to be UTF-8 without a BOM for all platforms. This is a breaking change as cmdlets on windows have a number of different encodings. This supports better interoperability with tradition Linux shells as we are using the same encoding.

Use ArgumentCompletionsAttribute for the parameter to accept the currently used strings.
Register encoding providers on all platforms. This seems safe enough, and there are defintely more encodings available on CoreFX after this is done, which will be useful for some of the web cmdlet scenarios

Validate that files are created with UTF-8 encoding without BOM
Update tests to validate Encoding parameter to new type and create new tests for
parameter type validation.

Breaking Change The -Encoding Byte has been removed from the filesystem provider cmdlets. A new parameter -Byte is now the signal that a byte stream is required as input or output will be a stream of bytes.

Show outdated Hide outdated src/System.Management.Automation/engine/Utils.cs
Show outdated Hide outdated src/System.Management.Automation/namespaces/FileSystemProvider.cs
Show outdated Hide outdated src/System.Management.Automation/namespaces/FileSystemProvider.cs
Show outdated Hide outdated src/System.Management.Automation/utils/ClrFacade.cs
Show outdated Hide outdated src/System.Management.Automation/utils/PathUtils.cs
Show outdated Hide outdated test/powershell/engine/Basic/Encoding.Tests.ps1
Show outdated Hide outdated test/powershell/engine/Basic/Encoding.Tests.ps1
${testStr} >> $outputFile
$bytes = Get-FileBytes $outputFile
$Expected = $( $ExpectedWithNewline; $ExpectedWithNewline )
$bytes -join "-" | should be ($Expected -join "-")

This comment has been minimized.

@adityapatwardhan

adityapatwardhan Oct 10, 2017

Member

Should this be BeExactly

@adityapatwardhan

adityapatwardhan Oct 10, 2017

Member

Should this be BeExactly

This comment has been minimized.

@JamesWTruher

JamesWTruher Oct 12, 2017

Member

nope - they're numbers so case doesn't provide value

@JamesWTruher

JamesWTruher Oct 12, 2017

Member

nope - they're numbers so case doesn't provide value

Show outdated Hide outdated test/powershell/engine/Basic/Encoding.Tests.ps1
Show outdated Hide outdated ...crosoft.PowerShell.Commands.Utility/commands/utility/Send-MailMessage.cs
@dantraMSFT

This comment has been minimized.

Show comment
Hide comment
@dantraMSFT

dantraMSFT Oct 10, 2017

Contributor
internal static class EncodingConversion

I think this class should be in its own CS file; it's not a path utility and discoverability is problematic.

In fact, I think this and the associated ArgumentToEncodingTransformationAttribute should be moved into an encoding file.

Contributor

dantraMSFT commented Oct 10, 2017

internal static class EncodingConversion

I think this class should be in its own CS file; it's not a path utility and discoverability is problematic.

In fact, I think this and the associated ArgumentToEncodingTransformationAttribute should be moved into an encoding file.

Show outdated Hide outdated src/System.Management.Automation/utils/PathUtils.cs
Show outdated Hide outdated src/Microsoft.PowerShell.Commands.Utility/commands/utility/CSVCommands.cs
Show outdated Hide outdated src/Microsoft.PowerShell.Commands.Utility/commands/utility/CSVCommands.cs
Show outdated Hide outdated src/System.Management.Automation/utils/PathUtils.cs
Show outdated Hide outdated test/powershell/Language/Parser/RedirectionOperator.Tests.ps1
Show outdated Hide outdated test/powershell/Language/Parser/RedirectionOperator.Tests.ps1
$testPath = Join-Path -Path $TestDrive -ChildPath 'TailWithEncoding.txt'
$content | Set-Content -Path $testPath -Encoding $encoding
$content | Set-Content -Path $testPath -Encoding $encodingName

This comment has been minimized.

@dantraMSFT

dantraMSFT Oct 11, 2017

Contributor

Should this test cover the ByteEncoding path? It doesn't appear to.

@dantraMSFT

dantraMSFT Oct 11, 2017

Contributor

Should this test cover the ByteEncoding path? It doesn't appear to.

This comment has been minimized.

@JamesWTruher

JamesWTruher Oct 13, 2017

Member

covered by the tests in engine/Basic

@JamesWTruher

JamesWTruher Oct 13, 2017

Member

covered by the tests in engine/Basic

Show outdated Hide outdated src/System.Management.Automation/utils/PathUtils.cs
Show outdated Hide outdated src/System.Management.Automation/utils/PathUtils.cs
/// Defines the values that can be supplied as the encoding parameter in the
/// FileSystemContentDynamicParametersBase class.
/// </summary>
public enum FileSystemCmdletProviderEncoding

This comment has been minimized.

@iSazonov

iSazonov Oct 11, 2017

Collaborator

It seems we shouldn't remove public API. Maybeuse [ObsoleteAttribute].

@iSazonov

iSazonov Oct 11, 2017

Collaborator

It seems we shouldn't remove public API. Maybeuse [ObsoleteAttribute].

This comment has been minimized.

@SteveL-MSFT

SteveL-MSFT Oct 17, 2017

Member

Jim, can you check if this API is in PowerShell Standard 3.0? This enum isn't used at all now, right? So leaving it in with Obsolete doesn't help.

@SteveL-MSFT

SteveL-MSFT Oct 17, 2017

Member

Jim, can you check if this API is in PowerShell Standard 3.0? This enum isn't used at all now, right? So leaving it in with Obsolete doesn't help.

uint currentAnsiCp = NativeMethods.GetACP();
s_defaultEncoding = Encoding.GetEncoding((int)currentAnsiCp);
#endif
s_defaultEncoding = new UTF8Encoding(false);

This comment has been minimized.

@iSazonov

iSazonov Oct 11, 2017

Collaborator

I don't like the change. Previously (discussion #3248, PR #3467) I fixed this for Windows because we're still very sensitive to the OEM encoding. We have tons applications in enterprises that doesn't work if we don't set the correct OEM encoding on the system. If MSFT wants to cut it off, do it first in Windows 10 kernel and get feedback.

@iSazonov

iSazonov Oct 11, 2017

Collaborator

I don't like the change. Previously (discussion #3248, PR #3467) I fixed this for Windows because we're still very sensitive to the OEM encoding. We have tons applications in enterprises that doesn't work if we don't set the correct OEM encoding on the system. If MSFT wants to cut it off, do it first in Windows 10 kernel and get feedback.

This comment has been minimized.

@JamesWTruher

JamesWTruher Oct 13, 2017

Member

@iSazonov where are tests which provide validation for the changes you made in PR3467? The discussion in #3248 doesn't seem to have any concrete examples of misbehavior, which I would really like to understand. As for your last, I would rather get feedback on breaking behavior outside of the Windows ship cycles.

@JamesWTruher

JamesWTruher Oct 13, 2017

Member

@iSazonov where are tests which provide validation for the changes you made in PR3467? The discussion in #3248 doesn't seem to have any concrete examples of misbehavior, which I would really like to understand. As for your last, I would rather get feedback on breaking behavior outside of the Windows ship cycles.

This comment has been minimized.

@SteveL-MSFT

SteveL-MSFT Oct 13, 2017

Member

@iSazonov, @PowerShell/powershell-committee has already decided to go with UTF8 (No BOM) as default. Can you give me a specific example where this is known to break? If OEM is needed, perhaps we could add a setting in the powershell json file but still default to utf8

@SteveL-MSFT

SteveL-MSFT Oct 13, 2017

Member

@iSazonov, @PowerShell/powershell-committee has already decided to go with UTF8 (No BOM) as default. Can you give me a specific example where this is known to break? If OEM is needed, perhaps we could add a setting in the powershell json file but still default to utf8

This comment has been minimized.

@iSazonov

iSazonov Oct 14, 2017

Collaborator

PR #3467#3467 haven't tests because (1) we did not have a full set of encoding tests in the time (2) we waited the encoding RFC (3) we only restore Windows PowerShell behavior by removing a breaking change.
About concrete examples. I speak about business applications. I have no idea how I can demonstrate it concrete. I understand that you hardly ever changed the OEM encoding on your computers. But ask yourself why are customers forced to do so? If MSTF wants to see it and understand it, it needs TAP with non-english enterprises. As I said earlier, the simple answer is that we have many business applications that only work with a specific OEM encoding and read/write files in OEM. PowerShell also works in this environment! The #3467 was caused by the fact that the scripts were no longer working and we needed to re-encode the files. The value of PowerShell Core disappears completely.

We can look at it on the other side. A year ago there was a categorical position that we should be backward compatible and that we should avoid making breaking changes as much as possible. The position was later changed so we want to facilitate the adaptation of UNIX users. This led to the creation of many incompatible changes. The current change from this series. These two principles have proved to be contradictory. We need to decide which one's in charge.

If the first means that we want to allow multiple Windows users to use PowerShell on Unix without much effort, and to get feedback to start the process of evolutionary convergence of platforms.

The second actually means that we're creating a brand new product that cannot be version 6, but must be PowerShell Core 1.0. It's fair to say that Windows users will have to migrate to a new product instead of using it directly. In this case, I agree that we can be ahead of the Windows 10 system and even its Notepad and use Utf8 by default like all other changes. Only then will it not be expected that the community will begin to use PowerShell Core extensively before version 3.
The second way is very dangerous. Microsoft can break the Windows PowerShell community and not build a new one-there are already well-known and popular alternatives (PHP, JS, Perl) that have proven their reliability and performance and have already formed a significant community. I've seen some examples of Microsoft already quickly destroying communities. The recovery took several years. It's sad. If it were my business, I would prefer to keep today's real customers, not future abstracts, and make the first step similar WSL like Windows PowerShell for Unix to capture a reliable bridgehead. Only then the community would be prepared to accept any breaking changes as understandable and necessary. Sorry for many words :-)

@iSazonov

iSazonov Oct 14, 2017

Collaborator

PR #3467#3467 haven't tests because (1) we did not have a full set of encoding tests in the time (2) we waited the encoding RFC (3) we only restore Windows PowerShell behavior by removing a breaking change.
About concrete examples. I speak about business applications. I have no idea how I can demonstrate it concrete. I understand that you hardly ever changed the OEM encoding on your computers. But ask yourself why are customers forced to do so? If MSTF wants to see it and understand it, it needs TAP with non-english enterprises. As I said earlier, the simple answer is that we have many business applications that only work with a specific OEM encoding and read/write files in OEM. PowerShell also works in this environment! The #3467 was caused by the fact that the scripts were no longer working and we needed to re-encode the files. The value of PowerShell Core disappears completely.

We can look at it on the other side. A year ago there was a categorical position that we should be backward compatible and that we should avoid making breaking changes as much as possible. The position was later changed so we want to facilitate the adaptation of UNIX users. This led to the creation of many incompatible changes. The current change from this series. These two principles have proved to be contradictory. We need to decide which one's in charge.

If the first means that we want to allow multiple Windows users to use PowerShell on Unix without much effort, and to get feedback to start the process of evolutionary convergence of platforms.

The second actually means that we're creating a brand new product that cannot be version 6, but must be PowerShell Core 1.0. It's fair to say that Windows users will have to migrate to a new product instead of using it directly. In this case, I agree that we can be ahead of the Windows 10 system and even its Notepad and use Utf8 by default like all other changes. Only then will it not be expected that the community will begin to use PowerShell Core extensively before version 3.
The second way is very dangerous. Microsoft can break the Windows PowerShell community and not build a new one-there are already well-known and popular alternatives (PHP, JS, Perl) that have proven their reliability and performance and have already formed a significant community. I've seen some examples of Microsoft already quickly destroying communities. The recovery took several years. It's sad. If it were my business, I would prefer to keep today's real customers, not future abstracts, and make the first step similar WSL like Windows PowerShell for Unix to capture a reliable bridgehead. Only then the community would be prepared to accept any breaking changes as understandable and necessary. Sorry for many words :-)

This comment has been minimized.

@SteveL-MSFT

SteveL-MSFT Oct 14, 2017

Member

It seems again that a viable option is to have a setting for powershell.runtimeconfig.json to have this be OEM for compat reasons.

@SteveL-MSFT

SteveL-MSFT Oct 14, 2017

Member

It seems again that a viable option is to have a setting for powershell.runtimeconfig.json to have this be OEM for compat reasons.

This comment has been minimized.

@mklement0

mklement0 Oct 15, 2017

Contributor

I haven't followed this closely, but what happened to the $PSDefaultEncoding = 'WindowsLegacy' proposal from the RFC?

@mklement0

mklement0 Oct 15, 2017

Contributor

I haven't followed this closely, but what happened to the $PSDefaultEncoding = 'WindowsLegacy' proposal from the RFC?

This comment has been minimized.

@mklement0

mklement0 Oct 15, 2017

Contributor

And, just to clarify: for backward compatibility, it's both OEM and "ANSI" code pages that must be considered.

@mklement0

mklement0 Oct 15, 2017

Contributor

And, just to clarify: for backward compatibility, it's both OEM and "ANSI" code pages that must be considered.

This comment has been minimized.

@SteveL-MSFT

SteveL-MSFT Oct 15, 2017

Member

@mklement0 the simplified version we scoped for 6.0.0 does not include WindowsLegacy

@SteveL-MSFT

SteveL-MSFT Oct 15, 2017

Member

@mklement0 the simplified version we scoped for 6.0.0 does not include WindowsLegacy

This comment has been minimized.

@SteveL-MSFT

SteveL-MSFT Oct 19, 2017

Member

@JamesWTruher can you open an issue to cover @iSazonov 's concern?

@SteveL-MSFT

SteveL-MSFT Oct 19, 2017

Member

@JamesWTruher can you open an issue to cover @iSazonov 's concern?

This comment has been minimized.

@JamesWTruher

JamesWTruher Oct 23, 2017

Member

@mklement0 WindowsLegacy was pulled waiting for customer need - opened #5204 to track.

@JamesWTruher

JamesWTruher Oct 23, 2017

Member

@mklement0 WindowsLegacy was pulled waiting for customer need - opened #5204 to track.

Show outdated Hide outdated src/System.Management.Automation/utils/PathUtils.cs
Show outdated Hide outdated src/System.Management.Automation/utils/PathUtils.cs
Show outdated Hide outdated src/System.Management.Automation/utils/PathUtils.cs
Show outdated Hide outdated src/Microsoft.PowerShell.Commands.Utility/commands/utility/CSVCommands.cs
Show outdated Hide outdated src/System.Management.Automation/utils/PathUtils.cs
uint currentAnsiCp = NativeMethods.GetACP();
s_defaultEncoding = Encoding.GetEncoding((int)currentAnsiCp);
#endif
s_defaultEncoding = new UTF8Encoding(false);

This comment has been minimized.

@SteveL-MSFT

SteveL-MSFT Oct 13, 2017

Member

@iSazonov, @PowerShell/powershell-committee has already decided to go with UTF8 (No BOM) as default. Can you give me a specific example where this is known to break? If OEM is needed, perhaps we could add a setting in the powershell json file but still default to utf8

@SteveL-MSFT

SteveL-MSFT Oct 13, 2017

Member

@iSazonov, @PowerShell/powershell-committee has already decided to go with UTF8 (No BOM) as default. Can you give me a specific example where this is known to break? If OEM is needed, perhaps we could add a setting in the powershell json file but still default to utf8

@JamesWTruher

This comment has been minimized.

Show comment
Hide comment
@JamesWTruher

JamesWTruher Oct 17, 2017

Member

@adityapatwardhan @dantraMSFT @SteveL-MSFT I've believe I've made updates that address your blocking issues

Member

JamesWTruher commented Oct 17, 2017

@adityapatwardhan @dantraMSFT @SteveL-MSFT I've believe I've made updates that address your blocking issues

@SteveL-MSFT

This comment has been minimized.

Show comment
Hide comment
@SteveL-MSFT

SteveL-MSFT Oct 18, 2017

Member

@PowerShell/powershell-committee reviewed this and has agreed on:

  1. name the parameter -AsByteArray
  2. output warning if -Encoding is used with -AsByteArray that Encoding is not being used
Member

SteveL-MSFT commented Oct 18, 2017

@PowerShell/powershell-committee reviewed this and has agreed on:

  1. name the parameter -AsByteArray
  2. output warning if -Encoding is used with -AsByteArray that Encoding is not being used
@mklement0

This comment has been minimized.

Show comment
Hide comment
@mklement0

mklement0 Oct 19, 2017

Contributor

A message from the COSP (Crying Over Spilt Milk) department:

output warning if -Encoding is used with -AsByteArray that Encoding is not being used

This results in an awkward user experience, because users will expect incompatible parameters to be in different parameter sets, with attempts to combine them resulting in an error, not a warning.

A step backward in usability, and a breaking change to boot - and no gain that I can discern.

I also agree with @iSazonov that forgoing compatibility with the legacy encoding behavior on Windows altogether will dismay many Windows users, especially those whose "ANSI" / OEM code pages aren't based on the Latin alphabet.

If the idea is to focus on the Unix crowd first, I see trouble as well: broken quoting, broken CLI, broken globbing, lack of && and ||, ...

Contributor

mklement0 commented Oct 19, 2017

A message from the COSP (Crying Over Spilt Milk) department:

output warning if -Encoding is used with -AsByteArray that Encoding is not being used

This results in an awkward user experience, because users will expect incompatible parameters to be in different parameter sets, with attempts to combine them resulting in an error, not a warning.

A step backward in usability, and a breaking change to boot - and no gain that I can discern.

I also agree with @iSazonov that forgoing compatibility with the legacy encoding behavior on Windows altogether will dismay many Windows users, especially those whose "ANSI" / OEM code pages aren't based on the Latin alphabet.

If the idea is to focus on the Unix crowd first, I see trouble as well: broken quoting, broken CLI, broken globbing, lack of && and ||, ...

@SteveL-MSFT

This comment has been minimized.

Show comment
Hide comment
@SteveL-MSFT

SteveL-MSFT Oct 19, 2017

Member

@mklement one of the considerations the committee acknowledged is the complexity of using multiple parametersets to make parameters mutually exclusive so we were ok with using a warning as a simplification until we can revisit the recalled RFC on making it easier to both define as well as understand mutually exclusive parameters

Member

SteveL-MSFT commented Oct 19, 2017

@mklement one of the considerations the committee acknowledged is the complexity of using multiple parametersets to make parameters mutually exclusive so we were ok with using a warning as a simplification until we can revisit the recalled RFC on making it easier to both define as well as understand mutually exclusive parameters

@daxian-dbw

I'm now looking at FileSystemProvider.cs, but want to share my feedback early.

Show outdated Hide outdated src/Microsoft.PowerShell.Commands.Utility/commands/utility/CSVCommands.cs
Show outdated Hide outdated src/Microsoft.PowerShell.Commands.Utility/commands/utility/CSVCommands.cs
Show outdated Hide outdated ...mmands.Utility/commands/utility/FormatAndOutput/format-hex/Format-Hex.cs
@@ -72,25 +73,9 @@ public string LiteralPath
/// </summary>
///
[Parameter(Position = 1)]
[ValidateNotNullOrEmpty]

This comment has been minimized.

@daxian-dbw

daxian-dbw Oct 19, 2017

Member

I think we still need this attribute, so that user cannot pass in a null value.

@daxian-dbw

daxian-dbw Oct 19, 2017

Member

I think we still need this attribute, so that user cannot pass in a null value.

@@ -206,8 +206,9 @@ public SwitchParameter NoClobber
/// Encoding optional flag
/// </summary>
[Parameter()]
[ValidateSetAttribute(new string[] { "Unicode", "UTF7", "UTF8", "ASCII", "UTF32", "BigEndianUnicode", "Default", "OEM" })]
public string Encoding { get; set; }
[ArgumentToEncodingTransformationAttribute()]

This comment has been minimized.

@daxian-dbw

daxian-dbw Oct 19, 2017

Member

I think we need to add AttributeNotNullOrEmpty attribute. Before this change, $null value would be rejected because the validate attribute will fail. After this change, user is able to pass in a null value.

@daxian-dbw

daxian-dbw Oct 19, 2017

Member

I think we need to add AttributeNotNullOrEmpty attribute. Before this change, $null value would be rejected because the validate attribute will fail. After this change, user is able to pass in a null value.

Show outdated Hide outdated src/Microsoft.PowerShell.Commands.Utility/commands/utility/MatchString.cs
Show outdated Hide outdated ...crosoft.PowerShell.Commands.Utility/commands/utility/Send-MailMessage.cs
Show outdated Hide outdated src/Microsoft.PowerShell.Commands.Utility/commands/utility/XmlCommands.cs
Show outdated Hide outdated src/Microsoft.PowerShell.Commands.Utility/commands/utility/XmlCommands.cs
@@ -1210,77 +1211,26 @@ internal static FileSystemCmdletProviderEncoding GetEncoding(string path)
string initialBytesAsAscii = System.Text.Encoding.ASCII.GetString(initialBytes, 0, bytesRead);
if (initialBytesAsAscii.IndexOfAny(nonPrintableCharacters) >= 0)
{
return FileSystemCmdletProviderEncoding.Byte;
return Encoding.Unicode;

This comment has been minimized.

@daxian-dbw

daxian-dbw Oct 19, 2017

Member

Returning Encoding.Unicode doesn't seem right. It looks we should deal with bytes in this case.

@daxian-dbw

daxian-dbw Oct 19, 2017

Member

Returning Encoding.Unicode doesn't seem right. It looks we should deal with bytes in this case.

This comment has been minimized.

@JamesWTruher

JamesWTruher Oct 20, 2017

Member

There is no byte encoding, and FileSystemCmdletProviderEncoding.Byte wound up being converted to a unicode encoding in GetEncodingFromEnum (now removed) so there's actually no change here.

@JamesWTruher

JamesWTruher Oct 20, 2017

Member

There is no byte encoding, and FileSystemCmdletProviderEncoding.Byte wound up being converted to a unicode encoding in GetEncodingFromEnum (now removed) so there's actually no change here.

@dantraMSFT

The change from property to field for some of the Encoding parameters needs to be reviewed for null references. MatchString.cs is one case where the parameter is a field, it is not mandatory, not marked as ValidateNotNull and is passed to an underlying StreamReader unchecked.

Show outdated Hide outdated src/Microsoft.PowerShell.Commands.Utility/commands/utility/MatchString.cs
@@ -1434,7 +1415,7 @@ private bool ProcessFile(string filename)
using (FileStream fs = new FileStream(filename, FileMode.Open, FileAccess.Read, FileShare.ReadWrite))
{
using (StreamReader sr = new StreamReader(fs, _textEncoding))
using (StreamReader sr = new StreamReader(fs, Encoding))

This comment has been minimized.

@dantraMSFT

dantraMSFT Oct 19, 2017

Contributor

There's an ArgumentNullException path here if Encoding is null.

@dantraMSFT

dantraMSFT Oct 19, 2017

Contributor

There's an ArgumentNullException path here if Encoding is null.

This comment has been minimized.

@JamesWTruher

JamesWTruher Oct 20, 2017

Member

fixed with [ValidateNotNullOrEmpty] attribute

@JamesWTruher

JamesWTruher Oct 20, 2017

Member

fixed with [ValidateNotNullOrEmpty] attribute

This comment has been minimized.

@JamesWTruher

JamesWTruher Oct 20, 2017

Member

fixed with attribute

@JamesWTruher

JamesWTruher Oct 20, 2017

Member

fixed with attribute

JamesWTruher added some commits Oct 2, 2017

Unify cmdlets with parameter 'Encoding' to be of type System.Text.Enc…
…oding

Created support classes so tab completion would continue to wor
Create an ArgumentTransformationAttribute for the parameter to accept the currently used strings
Register encoding providers on all platforms. This seems safe enough, and there are defintely more
encodings available on CoreFX after this is done, which will be useful for some of the web cmdlet scenarios

Validate that files are created with UTF-8 encoding without BOM
Update tests to validate Encoding parameter to new type and create new tests for
parameter type validation.
Change the encoding to utf8 when the converter gets an empty string
refactor encoding class and other encoding utilities into new file
update redirection operator tests to
convert string matching for encoding into dictionary, which should make it more efficient
Change Encoding to remove byte as a possible encoding value
Add a new parameter to the filesystem provider cmdlets to indicate whether we want a byte stream
Unify Send-MailMessage with other cmdlets
Changed -Byte parameter to -AsByteStream
Created a warning when both -AsByteStream and -Encoding are used together
updated tests to check that the warning is emitted
put ValidateNotNullOrEmpty attribute on Encoding parameters
Also improve encoding completion to handle wildcards
Also make sure that Encoding parameters are properties rather than fields.
Remove unused OpenStreamReader which takes a string since it's no longer in use
Change from IArgumentCompleter attribute to ArgumentCompletions attri…
…bute for encoding attribute.

Simplify logic for converting string to encoding
EncodingConversion.Utf8Bom,
EncodingConversion.Utf8NoBom,
EncodingConversion.Utf32
)]

This comment has been minimized.

@daxian-dbw

daxian-dbw Oct 23, 2017

Member

How about use ArgumentCompletions(EncodingConversion.TabCompletionResults) here? It would be easy for future updates too.

@daxian-dbw

daxian-dbw Oct 23, 2017

Member

How about use ArgumentCompletions(EncodingConversion.TabCompletionResults) here? It would be easy for future updates too.

This comment has been minimized.

@daxian-dbw

daxian-dbw Oct 23, 2017

Member

Same comments for other instances of ArgumentCompletions usage.

@daxian-dbw

daxian-dbw Oct 23, 2017

Member

Same comments for other instances of ArgumentCompletions usage.

This comment has been minimized.

@JamesWTruher

JamesWTruher Oct 23, 2017

Member

that doesn't work, because arrays aren't constant (I tried that first)

@JamesWTruher

JamesWTruher Oct 23, 2017

Member

that doesn't work, because arrays aren't constant (I tried that first)

This comment has been minimized.

@daxian-dbw

daxian-dbw Oct 23, 2017

Member

OK, then we are good here. #Closed

@daxian-dbw

daxian-dbw Oct 23, 2017

Member

OK, then we are good here. #Closed

@daxian-dbw

This comment has been minimized.

Show comment
Hide comment
@daxian-dbw

daxian-dbw Oct 23, 2017

Member

@dantraMSFT Can you please take another look and update your review?

Member

daxian-dbw commented Oct 23, 2017

@dantraMSFT Can you please take another look and update your review?

Comments has been addressed by later commits.

@daxian-dbw

This comment has been minimized.

Show comment
Hide comment
@daxian-dbw

daxian-dbw Oct 24, 2017

Member

I'm getting this PR merged so that it can be validated by today's daily builds, and hopefully, it can catch Beta.9 release.
Actually, this PR should have the last commit marked with '[Feature]' tag to trigger full build run.

Member

daxian-dbw commented Oct 24, 2017

I'm getting this PR merged so that it can be validated by today's daily builds, and hopefully, it can catch Beta.9 release.
Actually, this PR should have the last commit marked with '[Feature]' tag to trigger full build run.

@daxian-dbw daxian-dbw merged commit be70072 into PowerShell:master Oct 24, 2017

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla All CLA requirements met.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment