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

Add ANSI support for output #2199

Merged
merged 37 commits into from
Aug 11, 2022
Merged

Add ANSI support for output #2199

merged 37 commits into from
Aug 11, 2022

Conversation

fflaten
Copy link
Collaborator

@fflaten fflaten commented Jun 27, 2022

PR Summary

Introduces a configuration option Output.RenderMode and a wrapper-function to replace Write-Host so we can output strings using ANSI-escaped sequences to get colors in CI or plaintext etc. The option supports the following levels:

  • Auto (default):
    • Sets to Plaintext if $env:NO_COLOR
    • Sets to Ansi if $host.UI.SupportsVirtualTerminal
    • Else, Legacy (same as today)
  • Ansi
    • To force it. PSv3/v4 doesn't support auto-detect but CI might. Ex. our Azure Pipeline CI in this repo
  • Legacy (same as before this PR)
    • For those who want color in GUI, but not in CI or redirected output to logs etc.
  • Plaintext (why not.. https://no-color.org/)

The new function Write-PesterHostMessage supports all parameters of Write-Host to make it an easy replacement.

Fix #1953

TODO:

  • Fix auto-detection
    • V3/V4 compatible
    • PS6_2 run in CI should not use Ansi
  • Add tests for Write-PesterHostMessage
  • Add tests for config option and auto-detection
  • Test Azure DevOps and GitHub Actions
    • Fix multiline-strings
  • Test PSJobs (remoting)
  • Support custom PSHosts without UI
    • Works for PS3 and PS4 now. Output only available in PS5+ where Information-stream exists
  • Support NO_COLOR

PR Checklist

  • PR has meaningful title
  • Summary describes changes
  • PR is ready to be merged
    • If not, use the arrow next to Create Pull Request to mark it as a draft. PR can be marked Ready for review when it's ready.
  • Tests are added/update (if required)
  • Documentation is updated/added (if required)

@fflaten
Copy link
Collaborator Author

fflaten commented Jun 29, 2022

Alternative configuration using StringOption Output.RenderMode:

  • Auto (default):
    • Set to Plaintext if $env:NO_COLOR
    • Set to Ansi if $host.UI.SupportsVirtualTerminal
    • Else, default (ConsoleColor like today)
  • Ansi (force it, because PSv3/v4 doesn't support auto-detect but CI might (like our Azure Pipeline checks in this PR)
  • ConsoleColor (for those who want color in GUI, but not in CI or redirected output to logs etc)
  • Plaintext (idk.. https://no-color.org/)

@nohwnd @beatcracker @briantist Thoughts? Overcomplicating it? 😄

@briantist
Copy link

Alternative configuration using StringOption Output.RenderMode:

  • Auto (default):

    • Set to Plaintext if $env:NO_COLOR
    • Set to Ansi if $host.UI.SupportsVirtualTerminal
    • Else, default (ConsoleColor like today)
  • Ansi (force it, because PSv3/v4 doesn't support auto-detect but CI might (like our Azure Pipeline checks in this PR)

  • ConsoleColor (for those who want color in GUI, but not in CI or redirected output to logs etc)

  • Plaintext (idk.. https://no-color.org/)

@nohwnd @beatcracker @briantist Thoughts? Overcomplicating it? 😄

I like the options as you have them laid out here. It doesn't seem overcomplicated to me, it seems thorough and well thought out. I didn't know about no-color and I think it's a nice idea and should be supported.

@nohwnd
Copy link
Member

nohwnd commented Jun 30, 2022

The options seem more straightforward to me, if we add a "conflicting" FQFQ (or whatever) output option in the future, we don't have to solve what happens where there are both UseAnsi and UseFQFQ.

The breaking change:

Is writing via console directly necessary to implement this? Or is that just for perf?

@fflaten
Copy link
Collaborator Author

fflaten commented Jun 30, 2022

Is writing via console directly necessary to implement this? Or is that just for perf?

Perf. Regain the delay added by wrapper (and more).

I'm considering replacing option ConsoleColor with Legacy above and passthrough to write-host as before though somewhat slower due to wrapper overhead.

Then custom PSHosts could still use the information-output, even though it's still broken regarding -nonewline. Need to implement ugly array of dict-input to solve that and I don't really thinks it's worth it.

@fflaten
Copy link
Collaborator Author

fflaten commented Jun 30, 2022

Printing 1000 lines with ForegroundColor (average of 5 runs)

Method PowerShell 5.1 Pwsh 7.2.5 VSCode Integrated 5.1
Write-Host (reference) 450ms 217ms 660ms
New with ANSI and $host.UI 260ms 132ms 418ms
New without ANSI and $host.UI 323ms 210ms 500ms
New with ANSI and Write-Host 620ms 350ms 980ms
New without ANSI and Write-Host 580ms 290ms 830ms

We got 5x lines in pipeline, so ~2sec test-time per runner maybe. Maybe not worth it?

  • Passthru to Write-Host for Legacy (without ANSI) for backwards-compatibility, but $host.UI for ANSI?
  • Passthru for all to keep it simple (but slower than today)?

I'm fine either way, just thought we already had gotten some feedback on output being slow 🙂

The difference was bigger last year when I tried to support array-input to fix the broken information-stream at the same time.

@nohwnd
Copy link
Member

nohwnd commented Jul 1, 2022

I think sticking with Write-Host is the right thing to do here, even though it is a bit slower.

@fflaten fflaten marked this pull request as ready for review July 1, 2022 22:04
@fflaten
Copy link
Collaborator Author

fflaten commented Jul 1, 2022

Ready for review. Good thing we squash commits. 😄 Let me know if I should rebase

src/Main.ps1 Outdated Show resolved Hide resolved
src/functions/Output.ps1 Show resolved Hide resolved
@nohwnd
Copy link
Member

nohwnd commented Aug 3, 2022

Really nice, some small suggestions.

@nohwnd nohwnd merged commit 13fb4a2 into pester:main Aug 11, 2022
@nohwnd
Copy link
Member

nohwnd commented Aug 11, 2022

Merged, thanks for that.

@fflaten
Copy link
Collaborator Author

fflaten commented Aug 11, 2022

Thanks! Finally done with this conflict-magnet 😄 🎉

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.

Using ANSI escape codes for output
3 participants