Skip to content

Allow numeric Ids and name of registered code pages in '-Encoding' parameters - #7636

Merged
iSazonov merged 4 commits into
PowerShell:masterfrom
iSazonov:encoding-enhance
Sep 19, 2018
Merged

Allow numeric Ids and name of registered code pages in '-Encoding' parameters#7636
iSazonov merged 4 commits into
PowerShell:masterfrom
iSazonov:encoding-enhance

Conversation

@iSazonov

@iSazonov iSazonov commented Aug 27, 2018

Copy link
Copy Markdown
Collaborator

PR Summary

Address #6581.

The PR enhance the encoding transformation attribute:

  • to allow numeric Ids of registered code pages (like -Encoding 1251)
  • to allow string names of registered code pages (like -Encoding "windows-1251")

Full list supported code pages we can found in
https://github.com/dotnet/corefx/blob/0b0f63d47951773cc46393312d80b40176d3a413/src/System.Text.Encoding.CodePages/src/System/Text/EncodingTable.Data.cs

We haven't encoding test at all so I tested the change locally.

Set-Content -Path .\test1.txt -Value "я" -Encoding 1251
Set-Content -Path .\test2.txt -Value "я" -Encoding "windows-1251"

PR Checklist

@iSazonov

iSazonov commented Sep 10, 2018

Copy link
Copy Markdown
Collaborator Author

@TravisEz13 Could you please look why PowerShell-CIs failed on Update-Help tests? (Nightly Travis CI failed too).

@TravisEz13

Copy link
Copy Markdown
Member

there were connection issues to download.microsoft.com... The PR should be rebased.

@iSazonov

Copy link
Copy Markdown
Collaborator Author

Rebased to fix CIs.

{
return System.Text.Encoding.GetEncoding(stringName);
}
case int intName:

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add some tests for the int case?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added.

@TravisEz13 TravisEz13 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see comments

BeforeAll {
$testValue = "ф"
if ($IsWindows) {
$expectedBytes = 244,13,10 -join "-"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a comment about what the characters are

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added.

@TravisEz13 TravisEz13 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One comment about the expected characters

@iSazonov iSazonov added the Documentation Needed in this repo Documentation is needed in this repo label Sep 18, 2018
@TravisEz13

Copy link
Copy Markdown
Member

@iSazonov Have you filed a doc issue?

@iSazonov

Copy link
Copy Markdown
Collaborator Author

@TravisEz13 Doc issue was created.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

3 participants