-
Notifications
You must be signed in to change notification settings - Fork 7.1k
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
Make Out-String
and Out-File
keep string input unchanged
#17455
Conversation
We need to update docs https://docs.microsoft.com/en-us/powershell/module/microsoft.powershell.core/about/about_ansi_terminals?view=powershell-7.2
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thank you for making it still possible to retain formatting when output rendering is Ansi
❤️
@PowerShell/powershell-committee Need the committee to review the breaking change comparing with 7.2.x. In 7.2.x, When the passed-in object has formatting views applied to it, removing escape sequences from the resulted formatting text makes perfect sense when the output rendering option is However, when the passed-in objects are pure strings, those 2 cmdlets shouldn't make any changes to the string objects, because
I think the |
Is this going to get rid of the extra linebreak that happens too? |
@ImportTaste This PR only affects escape sequences. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really great change!
This PR has Quantification details
Why proper sizing of changes matters
Optimal pull request sizes drive a better predictable PR flow as they strike a
What can I do to optimize my changes
How to interpret the change counts in git diff output
Was this comment helpful? 👍 :ok_hand: :thumbsdown: (Email) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should seriously consider adding a parameter to out-string and out-file which retains our current behavior. Perhaps a parameter -AsPlainText
or some such will allow users to have the older behavior while we provide a safer default behavior.
I agree that we should consider so, but maybe wait until we get issue reports that someone really gets affected by this breaking change. |
@PowerShell/powershell-committee reviewed this, we would recommend adding a parameter to enable users to get back the previous behavior. |
@SteveL-MSFT Is that a requirement for this PR or could do in separate PR? Also, is that supposed to be done now or should wait until there is a report about needing the previous behavior? |
Given that the recommendation is a separate feature, I think it would be fine to use a separate PR if we really decided to add a parameter for the previous behavior. I will merge this PR and open an issue for that. Opened the issue #17509. I also left my comments there. |
🎉 Handy links: |
/backport to release/v7.2.6 |
Started backporting to release/v7.2.6: https://github.com/PowerShell/PowerShell/actions/runs/2784883343
|
@adityapatwardhan backporting to release/v7.2.6 failed, the patch most likely resulted in conflicts: $ git am --3way --ignore-whitespace --keep-non-patch changes.patch
Applying: Make Out-String and Out-File keep string input unchanged
.git/rebase-apply/patch:91: trailing whitespace.
(Get-Verb -Verb Get | Out-String).Contains("`e[") | Should -BeTrue
warning: 1 line adds whitespace errors.
Using index info to reconstruct a base tree...
M src/System.Management.Automation/FormatAndOutput/common/ILineOutput.cs
M test/powershell/engine/Formatting/PSStyle.Tests.ps1
Falling back to patching base and 3-way merge...
Auto-merging test/powershell/engine/Formatting/PSStyle.Tests.ps1
CONFLICT (content): Merge conflict in test/powershell/engine/Formatting/PSStyle.Tests.ps1
Auto-merging src/System.Management.Automation/FormatAndOutput/common/ILineOutput.cs
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Make Out-String and Out-File keep string input unchanged
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Error: The process '/usr/bin/git' failed with exit code 128 Please backport manually! |
Backport done via #17859 |
🎉 Handy links: |
…#17859) Co-authored-by: Dongbo Wang <dongbow@microsoft.com>
PR Summary
Fix #17452
The changes in this PR update the behavior of
Out-File
andOut-String
regarding theRenderingOutput
setting to be the following:RenderingOutput
settingRenderingOutput
setting.This change is a breaking change to these 2 cmdlets' existing behavior regarding
RenderingOutput
.PR Checklist
.h
,.cpp
,.cs
,.ps1
and.psm1
files have the correct copyright headerWIP:
or[ WIP ]
to the beginning of the title (theWIP
bot will keep its status check atPending
while the prefix is present) and remove the prefix when the PR is ready.Out-File
andOut-String
regarding$PSStyle.OutputRendering
MicrosoftDocs/PowerShell-Docs#8892