Skip to content

RecommendedAction: Explicitly start and stop ANSI Error Color #24065

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
andyleejordan merged 2 commits into from
Jul 29, 2024
Merged

RecommendedAction: Explicitly start and stop ANSI Error Color #24065

andyleejordan merged 2 commits into from
Jul 29, 2024

Conversation

@JustinGrote
Copy link
Contributor

@JustinGrote JustinGrote commented Jul 19, 2024 •
edited
Loading

CC @SteveL-MSFT please merge before 7.5 GA. Thanks.

PR Summary

Fixes a bug in #20826 where the recommended action does not get colorized in certain circumstances where the ANSI reset code is presented prior to the RecommendedAction test.

Before

image

After

image

PR Context

PR Checklist

@SteveL-MSFT SteveL-MSFT added WG-Interactive-Console the console experience WG-NeedsReview Needs a review by the labeled Working Group labels Jul 22, 2024
@SteveL-MSFT
Copy link
Member

SteveL-MSFT commented Jul 22, 2024

@theJasonHelmick can you have the interactive WG review? It may make sense not to use the error color as the recommendation isn't part of the error

@theJasonHelmick
Copy link
Collaborator

theJasonHelmick commented Jul 22, 2024

@theJasonHelmick can you have the interactive WG review? It may make sense not to use the error color as the recommendation isn't part of the error

Will review this week on Wednesday :)

@kilasuit kilasuit mentioned this pull request Jul 24, 2024
Closed
@kilasuit
Copy link
Collaborator

kilasuit commented Jul 24, 2024

I feel that a recomendation is akin to something that potentially should be passed into to the Informational Stream, so should not take on the colour from the error stream, even though it is a property of the error record.

Plus as it is, to me this is more likely to stand out and be acted on than lost in the noise of the rest of the error (especially if there are many of them in a session and for a long time most errors will not have recommended actions)

However, it took too long to try test this out with preview.3 not yet in the MSStore & the latest daily is well outdated as the process for releasing the daily build is currently still broken but by downloading the ZIP of preview.3 I could get to works like in the first image, which to me is how I would like this to be in future, and could be a potential $PSStyle addition if you wanted to change that colour to something else instead but that's not really a discussion for this PR but jotting it down for the I-UX WG to comment on further

kilasuit
kilasuit reviewed Jul 24, 2024
View reviewed changes
...stem.Management.Automation/FormatAndOutput/DefaultFormatters/PowerShellCore_format_ps1xml.cs
if (-not [String]::IsNullOrWhiteSpace($recommendedAction)) {
$recommendedAction = $newline + ' Recommendation: ' + $recommendedAction
$recommendedAction = $newline +
${errorColor} +
Copy link
Collaborator

@kilasuit kilasuit Jul 24, 2024

Choose a reason for hiding this comment

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

Suggestion in line with my other comment is that instead of ${errorColor} this could be ${recommendationColor} to allow for differing colours to be used instead.

Copy link
Contributor Author

@JustinGrote JustinGrote Jul 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@kilasuit adding a PSStyle option was discussed in the original PR, there's actually a lot that goes into adding that vs. keeping this a small simple PR, I can't commit I could get all of that done by 7.5 but I structured it such that it could be added in the future.

For now I still think it should be formatted with ErrorColor because it is a related property, and gives the user the ability to still customize with PSStyle (as per the original PR implementation and justification), the current behavior is a bug.

Copy link
Collaborator

@theJasonHelmick theJasonHelmick Jul 24, 2024

Choose a reason for hiding this comment

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

The WG Interactive group has reviewed this. We agree to implement as this pr describes using ErrorColor. We would recommend making this customizable in the future based on Recommended Action.

Copy link
Member

@andyleejordan andyleejordan Jul 29, 2024

Choose a reason for hiding this comment

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

Yup, all makes sense to me!

@theJasonHelmick theJasonHelmick added WG-Reviewed A Working Group has reviewed this and made a recommendation Issue-Bug Issue has been identified as a bug in the product CommunityDay-Small A small PR that the PS team has identified to prioritize to review and removed WG-NeedsReview Needs a review by the labeled Working Group labels Jul 24, 2024
@SteveL-MSFT
Copy link
Member

SteveL-MSFT commented Jul 29, 2024 •
edited by unfurl-links bot
Loading

However, it took too long to try test this out with preview.3 not yet in the MSStore

7.5-preview.3 has been in the MSStore https://www.microsoft.com/store/apps/9P95ZZKTNRN4. Are you not seeing the preview.3 version in your region?

Microsoft Apps
PowerShell is a task-based command-line shell and scripting language built on .NET. PowerShell helps system administrators and power-users rapidly automate tasks that manage operating systems (Linux, macOS, and Windows) and processes.

PowerShell commands let you manage computers from the command line. PowerShell providers let you access data stores, such as the registry and certificate store, as easily as you access the file system. PowerShell includes a rich expression parser and a fully developed scripting language.

PowerShell is Open Source. See https://github.com/powershell/powershell

SteveL-MSFT
SteveL-MSFT approved these changes Jul 29, 2024
View reviewed changes
Copy link
Member

@SteveL-MSFT SteveL-MSFT left a comment

Choose a reason for hiding this comment

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

Since the WG approved this design change, the code change LGTM

@JustinGrote
Copy link
Contributor Author

JustinGrote commented Jul 29, 2024 •
edited
Loading

Since the WG approved this design change, the code change LGTM

Just to clarify for @andyleejordan this isn't a design change, rather a bugfix to realize the original design intent in #20826

EDIT: Specifically #20826 (comment)

@andyleejordan andyleejordan merged commit 5ae0fbd into PowerShell:master Jul 29, 2024
@microsoft-github-policy-service
Copy link
Contributor

microsoft-github-policy-service bot commented Jul 29, 2024 •
edited by unfurl-links bot
Loading

📣 Hey @JustinGrote, how did we do? We would love to hear your feedback with the link below! 🗣️

🔗 https://aka.ms/PSRepoFeedback

@andyleejordan
Copy link
Member

andyleejordan commented Jul 29, 2024

Thanks @JustinGrote!

@JustinGrote JustinGrote deleted the justingrote/fix/recommendedActionColors branch July 30, 2024 00:17
@SeeminglyScience SeeminglyScience added the CL-Engine Indicates that a PR should be marked as an engine change in the Change Log label Aug 20, 2024
chrisdent-de pushed a commit to chrisdent-de/PowerShell that referenced this pull request Sep 12, 2024
@JustinGrote @chrisdent-de
RecommendedAction: Explicitly start and stop ANSI Error Color (PowerS…
f0bae0e 
…hell#24065)
@iRon7 iRon7 mentioned this pull request Feb 21, 2025
Closed
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@theJasonHelmick theJasonHelmick theJasonHelmick left review comments

@andyleejordan andyleejordan andyleejordan approved these changes

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

+1 more reviewer

@kilasuit kilasuit kilasuit left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@andyleejordan andyleejordan

Labels

CL-Engine Indicates that a PR should be marked as an engine change in the Change Log CommunityDay-Small A small PR that the PS team has identified to prioritize to review Issue-Bug Issue has been identified as a bug in the product WG-Interactive-Console the console experience WG-Reviewed A Working Group has reviewed this and made a recommendation

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@JustinGrote @SteveL-MSFT @theJasonHelmick @kilasuit @andyleejordan @SeeminglyScience