Fix help paging issues on macOS/Linux and with custom pager that takes arguments#9033
Conversation
This fixes PowerShell#7175 by using Out-String to properly wrap text before sending to less or the user specified PAGER.
c29f5d7 to
790d2e3
Compare
| # Tokenize the specified $env:PAGER value. Ignore tokenizing errors since any errors may be valid | ||
| # argument syntax for the paging utility. | ||
| $errs = $null | ||
| $tokens = [System.Management.Automation.PSParser]::Tokenize($env:PAGER, [ref]$errs) |
There was a problem hiding this comment.
I'd expect that Tokenizer is very fast and more fast than Get-Command. So we could always call it and then Get-Command. This simplify code.
We need look $errs and write useful message to user.
Also we could use try-catch for Get-Command and running the command to write more useful messages to user.
There was a problem hiding this comment.
I'm not sure we should expose tokenizer errors. We are only using the tokenizer when piping help output to applications. The arguments passed to those applications could very well fail to tokenize by PowerShell. For instance, say an app took an argument like @{HEAD-1}, perfectly valide for the app but gens a tokenizer error:
Missing '=' operator after key in hash literal.
For debugging, I propose we leave the __PSPAGER_ARGS env var so the user can inspect it to see exactly what args we are passing to a pager application.
There was a problem hiding this comment.
I'm not sure we should expose tokenizer errors.
No expose. I think we could use this to create more useful message to user if it is possible.
|
@RDIL all of those are within a string which is actually PowerShell embedded into the C# code, so they should follow PowerShell style. :) |
|
If for nothing else, because it'll help you pick all the nuances up that much faster! 🙂 I had to triple check that, too, because something didn't look quite right, so don't feel bad. I stared at it for a good ten minutes before I realized what was going on. 😄 |
|
I suppose it is unlikely that this will make it in for the 6.2 release, eh? |
|
Yes, GA-6.2 was already forked. In next milestone perhaps we'll convert the function to cmdlet. |
|
@rkeithhill Sorry about this one. I see your PR will fix the issue I introduced. |
|
This PR has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed if no further activity occurs within 10 days. |
|
This PR needs some more review and to be targeted for some milestone - 6.2.1 or 7. I do not wish to see this closed as it addresses a real issue. |
| $customPagerCommand = $tokens[0].Content | ||
| if (!(Get-Command $customPagerCommand -ErrorAction Ignore)) { | ||
| # Custom pager command is invalid, issue a warning. | ||
| Write-Warning ""Ignoring invalid custom-paging utility command line specified in `$env:PAGER: $env:PAGER"" |
There was a problem hiding this comment.
This is more that it was not found? Can we update the message?
There was a problem hiding this comment.
I can update the message. How about: ""Custom-paging utility command not found. Ignoring command specified in `$env:PAGER: $env:PAGER""
There was a problem hiding this comment.
Only one minor issue. Tell me if you want me to address it before I merge.
|
@rkeithhill Thanks for fixing the paging issues. |
PR Summary
Fix #7175 by using
Out-String -Stream -Widthbefore piping to an application. This improves the help experience on macOS and Linux by ensuring lines of help text wrap properly for the given console width.Fix #8912 by using the PS tokenizer to separate the pager command from arguments if both are specified in $env:PAGER. Also ensures that there is no PowerShell interpretation of arguments to an application by using the stop parsing operator.
PR Context
The current reading experience of all help topics on macOS and Linux is sub-par giving PowerShell Core a bit of a black eye on those platforms. See #7175 for images of what this looks like.
Specify a custom pager with arguments works in v6.1 but is broken in v6.2. This PR fixes this issue - #8912.
PR Checklist
.h,.cpp,.cs,.ps1and.psm1files have the correct copyright headerWIP:or[ WIP ]to the beginning of the title (theWIPbot will keep its status check atPendingwhile the prefix is present) and remove the prefix when the PR is ready.[feature]to your commit messages if the change is significant or affects feature tests