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

Fix red #3325

Merged
merged 4 commits into from
Feb 4, 2022
Merged

Fix red #3325

merged 4 commits into from
Feb 4, 2022

Conversation

nohwnd
Copy link
Member

@nohwnd nohwnd commented Feb 4, 2022

Use Write-Host instead of setting output color and writing output.
Lock when outputting from callbacks, because they happen on thread pool and can run at the same time. Which ends up setting foreground color to red permanently.

scripts/common.lib.ps1 Outdated Show resolved Hide resolved
@Haplois Haplois enabled auto-merge (squash) February 4, 2022 11:08
if ($message)
{
$color = if ("Success" -eq $Level) { "Green" } else { "Red" }
Write-Host "... $message" -ForegroundColor $color
Copy link
Member

Choose a reason for hiding this comment

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

I am looking for the source but I remember to have read somewhere that it was better to avoid Write-Host and always use Write-Output/Write-Information/Write-Error instead.

Copy link
Member

Choose a reason for hiding this comment

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

Nevermind the articles are from 2013, I can't find any new article so that's probably obsolete remark.

Copy link
Member Author

Choose a reason for hiding this comment

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

That used to be the gospel. But it was never true anyway. But still it was repeated thousands of times, every time you shown someone a script with Write-Host in it :D

In this case using Write-Host or Write-Output surrounded by setting the output color do in fact have a slightly different meaning and behavior, and we are losing the ability to capture the output of Write-Host just by assigning the result of this script into a variable (but if we tried enough there is new set of recent cmdlets that Write-Host goes through anyway so we now have a way to capture even that). But because we don't plan to do anything like capturing all our output, and because we write to console directly from our process output handlers, this change is absolutely safe to do.

@Haplois Haplois merged commit f33445b into microsoft:main Feb 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants