-
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
Change default to OutputRendering.Host
and remove OutputRendering.Automatic
#15882
Conversation
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 found follow pattern twice in DecoratedString.cs:
if (outputRendering == OutputRendering.Automatic)
{
outputRendering = OutputRendering.Ansi;
Should we fix these too?
Formally it is a breaking change. |
The other usage of OutputRendering.Ansi is expected. |
@SteveL-MSFT Since now I may not fully understand the purpose of this change :) So, it looks like |
Yeah, we should probably get rid of |
OutputRendering.Host
and remove OutputRendering.Automatic
What if we want to change the default behavior for different systems after receiving feedback? |
@iSazonov perhaps not "ideal", but we can always add a |
src/System.Management.Automation/FormatAndOutput/common/StringDecorated.cs
Outdated
Show resolved
Hide resolved
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
Open PRs should not be assigned to milestone, so they are not assigned to the wrong milestone after they are merged. For backport consideration, use a |
{ | ||
return _text; | ||
throw new ArgumentException(StringDecoratedStrings.RequireExplicitRendering); |
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.
It is not good to throw in public ToString().
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.
If you take a closer look, you will find it won't throw in ToString()
. The exception could happen only when user calling ToString(OutputRendering.Host)
.
🎉 Handy links: |
🎉 Handy links: |
PR Summary
The original behavior of
Automatic
was borrowed from Linux where redirection kept ANSI codes and you would need to explicitly suppress it. macOS, on the other hand, did the opposite. I think for most users, particularly Windows users, it would be better to default toHost
so that ANSI is only written to the host and never to the pipeline. This will reduce confusion for users when they redirect text to a file and see ANSI codes in it. Users who want to keep the ANSI codes can always use$PSStyle.OutputRendering = 'Ansi'
to force it.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.(which runs in a different PS Host).