Skip to content

Make property names for the color VT sequences consistent with documentations #16212

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
adityapatwardhan merged 3 commits into from
Oct 11, 2021
Merged

Make property names for the color VT sequences consistent with documentations #16212

adityapatwardhan merged 3 commits into from
Oct 11, 2021

Conversation

@daxian-dbw
Copy link
Member

@daxian-dbw daxian-dbw commented Oct 8, 2021 •
edited
Loading

PR Summary

We use the property names 'Light***' such as LightRed and LightGreen for VT sequences that are documented as "Bright Foreground <color>" and "Bright Background <color>", such as 'Bright Red' and 'Bright Green'. This is misleading, we should make the property names consistent with those in the documentation, so it's more intuitive for users to work with VT color in PowerShell.

PowerShell/src/System.Management.Automation/FormatAndOutput/common/PSStyle.cs

Lines 78 to 88 in e98a8c8

public string LightBlue { get; } = "\x1b[94m";
/// <summary>
/// Gets the color light cyan.
/// </summary>
public string LightCyan { get; } = "\x1b[96m";
/// <summary>
/// Gets the color light gray.
/// </summary>
public string LightGray { get; } = "\x1b[97m";

More Context

The names were originally copied from the "command-line-api" repo from System.CommandLine.Rendering.Color+Foreground and System.CommandLine.Rendering.Color+Background. Now it's been confirmed that those properties were misnamed in that repo and will be fixed there, see dotnet/command-line-api#1438.

So we should fix in PowerShell as well, before 7.2 GA.

PR Checklist

@SteveL-MSFT SteveL-MSFT added Breaking-Change breaking change that may affect users WG-DevEx-SDK hosting PowerShell as a runtime, PowerShell's APIs, PowerShell Standard, or development of modules labels Oct 8, 2021
bergmeister
bergmeister approved these changes Oct 8, 2021
View reviewed changes
Copy link
Contributor

@bergmeister bergmeister left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looks good to me and could not spot a mistake. Due to the property names being public, it is breaking a change as already indicated but I don't think real software depends on it, I imagine only custom user setup. In theory we could leave the old property names and point them to the new ones and send telemetry.

@JustinGrote
Copy link
Contributor

JustinGrote commented Oct 8, 2021

Makes sense to me, since it is still preview and anyone working at this level is going to be used to these VT level color sequences, the consistency is appropriate.

@rkeithhill
Copy link
Collaborator

rkeithhill commented Oct 8, 2021

The $PSStyle is only in 7.2 previews, right? I don't see it in my 7.1.x install. So is this really a breaking change? IMO you have the right to change anything in a preview. That's why it's called "preview". Once it hits RC or especially any sort of "go live" designation, then you shouldn't introduce a breaking change unless it meets a very high bar.

@SteveL-MSFT
Copy link
Member

SteveL-MSFT commented Oct 8, 2021

@rkeithhill it was added in 7.2, we made it stable in preview.10 (or 9?). My opinion is to either change it now or never. I'm leaving it to the WG to make a formal decision :)

@daxian-dbw
Copy link
Member Author

daxian-dbw commented Oct 9, 2021

I will need to reorder the properties, will push another commit soon.

@rkeithhill
Copy link
Collaborator

rkeithhill commented Oct 9, 2021

Better do it now then IMO.

@daxian-dbw
Copy link
Member Author

daxian-dbw commented Oct 9, 2021

The names were originally copied from the "command-line-api" repo from System.CommandLine.Rendering.Color+Foreground and System.CommandLine.Rendering.Color+Background. Now it's been confirmed that those properties were misnamed in that repo and will be fixed there, see dotnet/command-line-api#1438.

So we should fix in PowerShell as well, before 7.2 GA.

@daxian-dbw daxian-dbw mentioned this pull request Oct 9, 2021
Merged
24 tasks
SteveL-MSFT
SteveL-MSFT requested changes Oct 9, 2021
View reviewed changes
@ghost ghost added the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label Oct 9, 2021
@ghost ghost removed the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label Oct 9, 2021
@daxian-dbw daxian-dbw added Backport-7.2.x-Consider CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log labels Oct 11, 2021
@adityapatwardhan adityapatwardhan merged commit c8984c0 into PowerShell:master Oct 11, 2021
@daxian-dbw daxian-dbw deleted the name branch October 11, 2021 22:00
daxian-dbw added a commit to daxian-dbw/PowerShell that referenced this pull request Oct 21, 2021
@daxian-dbw
Make property names for the color VT sequences consistent with docume…
93a3788 
…ntations (PowerShell#16212)
@ghost
Copy link

ghost commented Oct 21, 2021

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

Handy links:

@ghost
Copy link

ghost commented Dec 16, 2021

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

Handy links:

TrapGodBrim pushed a commit to TrapGodBrim/PowerShell that referenced this pull request Jan 19, 2022
@daxian-dbw @TrapGodBrim
Make property names for the color VT sequences consistent with docume…
a9514fa 
…ntations (PowerShell#16212)
@TravisEz13 TravisEz13 mentioned this pull request May 24, 2022
Merged
@TravisEz13 TravisEz13 mentioned this pull request Jun 7, 2022
Merged
22 tasks
@TravisEz13 TravisEz13 mentioned this pull request Apr 16, 2024
Closed
@TravisEz13 TravisEz13 mentioned this pull request Jul 9, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@SteveL-MSFT SteveL-MSFT SteveL-MSFT approved these changes

@adityapatwardhan adityapatwardhan adityapatwardhan approved these changes

@anmenaga anmenaga Awaiting requested review from anmenaga

@rjmholt rjmholt Awaiting requested review from rjmholt

@TravisEz13 TravisEz13 Awaiting requested review from TravisEz13

+1 more reviewer

@bergmeister bergmeister bergmeister approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@adityapatwardhan adityapatwardhan

Labels

Backport-7.2.x-Done Breaking-Change breaking change that may affect users CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log WG-DevEx-SDK hosting PowerShell as a runtime, PowerShell's APIs, PowerShell Standard, or development of modules

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@daxian-dbw @JustinGrote @rkeithhill @SteveL-MSFT @bergmeister @adityapatwardhan @TravisEz13