Skip to content

Conversation

@iSazonov
Copy link
Collaborator

@iSazonov iSazonov commented Aug 27, 2018

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
Copy link
Collaborator Author

iSazonov commented Sep 10, 2018

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

@TravisEz13
Copy link
Member

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

@iSazonov
Copy link
Collaborator Author

Rebased to fix CIs.

{
return System.Text.Encoding.GetEncoding(stringName);
}
case int intName:
Copy link
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
Collaborator Author

Choose a reason for hiding this comment

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

Added.

Copy link
Member

@TravisEz13 TravisEz13 left a comment

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
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
Collaborator Author

Choose a reason for hiding this comment

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

Added.

Copy link
Member

@TravisEz13 TravisEz13 left a comment

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
Member

@iSazonov Have you filed a doc issue?

@iSazonov
Copy link
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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants