Skip to content
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

Merged
merged 9 commits into from
Aug 16, 2021

Conversation

SteveL-MSFT
Copy link
Member

@SteveL-MSFT SteveL-MSFT commented Aug 5, 2021

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 to Host 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

Copy link
Collaborator

@iSazonov iSazonov left a 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?

test.txt Outdated Show resolved Hide resolved
@iSazonov iSazonov added Breaking-Change breaking change that may affect users CL-BreakingChange Indicates that a PR should be marked as a breaking change in the Change Log CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log labels Aug 6, 2021
@iSazonov
Copy link
Collaborator

iSazonov commented Aug 6, 2021

Formally it is a breaking change.

@SteveL-MSFT
Copy link
Member Author

The other usage of OutputRendering.Ansi is expected.

@daxian-dbw
Copy link
Member

daxian-dbw commented Aug 14, 2021

@SteveL-MSFT Since now Automatic is default to Host, any reason to keep OutputRendering.Automatic? Could we just use Host as the default value for $PSStyle.OutputRendering?

I may not fully understand the purpose of this change :) So, it looks like Automatic behaves like Host when rendering, but behaves like Ansi in StringDecorated/ValueStringDecorated.ToString. This inconsistency looks a little weird ...

@SteveL-MSFT
Copy link
Member Author

Yeah, we should probably get rid of Automatic now since it works the same as Host. I'll make that change.

@SteveL-MSFT SteveL-MSFT changed the title Change OutputRendering.Automatic (default) to behave as OutputRendering.Host Change default to OutputRendering.Host and remove OutputRendering.Automatic Aug 15, 2021
@SteveL-MSFT SteveL-MSFT changed the title Change default to OutputRendering.Host and remove OutputRendering.Automatic Change default to OutputRendering.Host and remove OutputRendering.Automatic Aug 15, 2021
@iSazonov
Copy link
Collaborator

What if we want to change the default behavior for different systems after receiving feedback?

@SteveL-MSFT
Copy link
Member Author

@iSazonov perhaps not "ideal", but we can always add a Automatic back later (it would not have enum value 0 though), but I'm convinced now that macOS got it right and Linux is just maintaining compat.

@ghost ghost added Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept and removed Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept labels Aug 16, 2021
Copy link
Member

@daxian-dbw daxian-dbw left a comment

Choose a reason for hiding this comment

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

LGTM

@daxian-dbw daxian-dbw added this to the 7.2.0-preview.9 milestone Aug 16, 2021
@ghost ghost removed this from the 7.2.0-preview.9 milestone Aug 16, 2021
@ghost
Copy link

ghost commented Aug 16, 2021

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 backport label.

@daxian-dbw daxian-dbw merged commit a162856 into PowerShell:master Aug 16, 2021
@SteveL-MSFT SteveL-MSFT deleted the ansi-host branch August 16, 2021 21:36
{
return _text;
throw new ArgumentException(StringDecoratedStrings.RequireExplicitRendering);
Copy link
Collaborator

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().

Copy link
Member

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).

@ghost
Copy link

ghost commented Aug 23, 2021

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

Handy links:

@ghost
Copy link

ghost commented Sep 28, 2021

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

Handy links:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Breaking-Change breaking change that may affect users CL-BreakingChange Indicates that a PR should be marked as a breaking change in the Change Log CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log WG-Interactive-Console the console experience
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants