Skip to content

Change default fallback encoding for GetEncoding in Start-Transcript #13732

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.

Sign up for GitHub

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

Jump to bottom
Merged
rjmholt merged 3 commits into from
Oct 9, 2020
Merged

Change default fallback encoding for GetEncoding in Start-Transcript #13732

rjmholt merged 3 commits into from
Oct 9, 2020

Conversation

@Gimly
Copy link
Contributor

@Gimly Gimly commented Oct 1, 2020 •
edited
Loading

PR Summary

The System.Management.Automation.Utils.GetEncoding method should return UTF8 without BOM as a default but was returning ASCII.

This is fixed by returning an instance of the Utf8Encoding with the BOM set to false as a default fallback encoding.

PR Context

This should fix #13678 and fix partially #13677

PR Checklist

@Gimly
Change default encoding for GetEncoding
d50faf2 
The System.Management.Automation.Utils.GetEncoding method
should return UTF8 without BOM as a default but was returning ASCII.

This is fixed by returning an instance of the Utf8Encoding with the BOM
set to false.
@ghost ghost assigned rjmholt Oct 1, 2020
@Gimly Gimly mentioned this pull request Oct 1, 2020
Closed
@rjmholt rjmholt requested review from JamesWTruher and daxian-dbw and removed request for daxian-dbw October 1, 2020 21:02
@rjmholt rjmholt added Breaking-Change breaking change that may affect users Review - Committee The PR/Issue needs a review from the PowerShell Committee labels Oct 1, 2020
@rjmholt
Copy link
Collaborator

rjmholt commented Oct 1, 2020 •
edited
Loading

@PowerShell/powershell-committee please review this potentially breaking change, noting the linked issues

rjmholt
rjmholt reviewed Oct 1, 2020
View reviewed changes
@Gimly @iSazonov
Update src/System.Management.Automation/engine/Utils.cs
f098c4f 
Add the label for the boolean argument

Co-authored-by: Ilya <darpa@yandex.ru>
@iSazonov
Copy link
Collaborator

iSazonov commented Oct 2, 2020

For PowerShell-Committee:

  • it affects only transcribe output files.
  • we already changed defaults to Uft8NoBOM in other code paths
  • I think it is a bug and we shouldn't consider this as a breaking change - it is unexpected behavior if we change encoding of existent file.

@SteveL-MSFT SteveL-MSFT changed the title Change default fallback encoding for GetEncoding in Start-Transcribe Change default fallback encoding for GetEncoding in Start-Transcript Oct 7, 2020
@SteveL-MSFT
Copy link
Member

SteveL-MSFT commented Oct 7, 2020

ASCII being a subset of UTF8 makes this an unlikely impactful breaking change (bucket 3) for me. Seems like the right thing to do.

@SteveL-MSFT
Copy link
Member

SteveL-MSFT commented Oct 7, 2020

@PowerShell/powershell-committee reviewed this, we agree that this is a bucket 3 breaking change that is unlikely to have negative impact on anyone. This should have been changed along with the other encoding changes to UTF-8 by default and is a bug fix.

@SteveL-MSFT SteveL-MSFT added Committee-Reviewed PS-Committee has reviewed this and made a decision and removed Review - Committee The PR/Issue needs a review from the PowerShell Committee labels Oct 7, 2020
daxian-dbw
daxian-dbw reviewed Oct 7, 2020
View reviewed changes
@rjmholt rjmholt added the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label Oct 7, 2020
@ghost ghost removed the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label Oct 8, 2020
@rjmholt
Copy link
Collaborator

rjmholt commented Oct 8, 2020

@daxian-dbw please re-review if you can

@Gimly
Copy link
Contributor Author

Gimly commented Oct 9, 2020

@rjmholt I'm not sure about those CI fails, do I need to do something about them?

@iSazonov
Copy link
Collaborator

iSazonov commented Oct 9, 2020

I restarted CI Windows.

@rjmholt rjmholt merged commit cf251a9 into PowerShell:master Oct 9, 2020
@rjmholt rjmholt added the CL-Engine Indicates that a PR should be marked as an engine change in the Change Log label Oct 9, 2020
@iSazonov
Copy link
Collaborator

iSazonov commented Oct 9, 2020

@Gimly Thanks for your contribution!

@Gimly Gimly deleted the default_utf8_in_starttranscript branch October 28, 2020 11:48
@ghost
Copy link

ghost commented Nov 17, 2020

🎉v7.2.0-preview.1 has been released which incorporates this pull request.:tada:

Handy links:

@ghost ghost mentioned this pull request Nov 17, 2020
Closed
@sdwheeler sdwheeler mentioned this pull request Jan 25, 2021
Closed
Steg17
Steg17 reviewed Mar 11, 2021
View reviewed changes
Copy link

@Steg17 Steg17 left a comment

Choose a reason for hiding this comment

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

:)

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

Reviewers

@daxian-dbw daxian-dbw daxian-dbw left review comments

@iSazonov iSazonov iSazonov approved these changes

@JamesWTruher JamesWTruher Awaiting requested review from JamesWTruher

+2 more reviewers

@Steg17 Steg17 Steg17 left review comments

@rjmholt rjmholt rjmholt approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@rjmholt rjmholt

Labels

Breaking-Change breaking change that may affect users CL-Engine Indicates that a PR should be marked as an engine change in the Change Log Committee-Reviewed PS-Committee has reviewed this and made a decision

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Start-Transcript -Append uses lossy ASCII character encoding when appending to a BOM-less target file

6 participants

@Gimly @rjmholt @iSazonov @SteveL-MSFT @daxian-dbw @Steg17