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 UseNullPSHostUI config so apps hosting PSES can disable it #2122

Merged

Conversation

dkattan
Copy link
Contributor

@dkattan dkattan commented Dec 26, 2023

PR Summary

Meaningful output from PSES's internal PowerShell is suppressed if you don't specify -EnableConsoleRepl.

PR Context

I'd like to be able to see the output from PSScriptAnalyzer but my application has a read-only terminal.
This SwitchParameter prevents the UI from being set to NullPSHostUI without enabling Repl functionality.

Perhaps a better way to implement this would be to condition off the use of named pipes as I believe the intention of the following code is to prevent clobbering stdio since ConsoleRepl can only be used when connecting via named pipes.

     UI = hostInfo.ConsoleReplEnabled || hostInfo.UseCurrentPSHostUI
                ? new EditorServicesConsolePSHostUserInterface(loggerFactory, hostInfo.PSHost.UI)
                : new NullPSHostUI();

@dkattan dkattan requested review from a team as code owners December 26, 2023 11:33
Copy link
Member

@andyleejordan andyleejordan left a comment

Choose a reason for hiding this comment

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

I like quite a bit of this but would want to clean it up a bit, put it through some thorough testing, and I think you are quite right that we should condition this based on named pipes instead of adding (yet another) parameter to the startup script. Let me know if you want to do some of that, and please tell me about the best way to go about testing this!

@dkattan
Copy link
Contributor Author

dkattan commented Jan 3, 2024

I like quite a bit of this but would want to clean it up a bit, put it through some thorough testing, and I think you are quite right that we should condition this based on named pipes instead of adding (yet another) parameter to the startup script. Let me know if you want to do some of that, and please tell me about the best way to go about testing this!

Sweet, I'll go ahead and make that change

@dkattan dkattan force-pushed the feature/bring-your-own-pshost-ui branch 7 times, most recently from 46df818 to e1e3bf2 Compare January 4, 2024 01:26
@dkattan
Copy link
Contributor Author

dkattan commented Jan 4, 2024

please tell me about the best way to go about testing this!

This is going to be hard to test since it appears that all the tests use Stdio, unless I’m mistaken.

@andyleejordan andyleejordan changed the title Show errors from PSScriptAnalyzer and other operations when -EnableConsoleRepl:$false Add UseNullPSHostUI config so apps hosting PSES can disable it Feb 12, 2024
@andyleejordan
Copy link
Member

That CI failure was a red herring, it was actually caused by GitHub Actions giving us a mix of PowerShell versions leading to a missed bug in a unit test, fixed by #2135

@andyleejordan andyleejordan added the Issue-Enhancement A feature request (enhancement). label Feb 12, 2024
@andyleejordan andyleejordan added this pull request to the merge queue Feb 13, 2024
Merged via the queue into PowerShell:main with commit e4b2fc4 Feb 13, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Issue-Enhancement A feature request (enhancement).
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

2 participants