Remove 'more' function and move the $env:PAGER capability into the help function #6059

iSazonov · 2018-01-29T10:59:28Z

PR Summary

Address #1267.
Fix #6497.

Based on PowerShell Committee conclusion (see below) and PR Native pipe #2450 we remove more function. Now users is free to use preffered pager directly or create a custom alias.
To keep current help function behavior we move a pager capability (with $env:PAGER support) to the function.

~~Before the change we had to wait 10 seconds before we could see the script output:~~
~~& { 1..100; sleep -seconds 10; 101..200 } | more~~
~~With this change we will see the output without delay.~~

~~I have no idea how to test this.~~

PR Checklist

Note: Please mark anything not applicable to this PR NA.

PR has a meaningful title
- Use the present tense and imperative mood when describing your changes
Summarized changes
[NA] User facing Documentation needed
- [NA] Issue filed - Issue link:
Change is not breaking
Make sure you've added a new test if existing tests do not effectively test the code changed
- Add [feature] if the change is significant or affects feature tests
This PR is ready to merge and is not Work in Progress.
- If the PR is work in progress, please add the prefix WIP: to the beginning of the title and remove the prefix when the PR is ready.

iSazonov · 2018-01-30T15:25:13Z

Please review. I think I did everything I could. Some Help tests failed - looks as breaking change. Also see Get-Process | more.

daxian-dbw · 2018-03-14T16:53:27Z

src/System.Management.Automation/engine/InitialSessionState.cs

+
+    $process.StartInfo.FileName = 'C:\Windows\system32\more.com'
+    $process.StartInfo.UseShellExecute = $false
+    $process.StartInfo.RedirectStandardInput = $true


The design is apparently not cross-plat. Please revisit the design and make sure it works on all supported platforms.

BrucePay

Do we still need the more function? I thought @vors fixed the pipeline so that objects would be streamed into executables. Certainly
& { 1..100; sleep -seconds 10; 101..200 } | more
is slow, but
& { 1..100; sleep -seconds 10; 101..200 } | more.com
and
& { 1..100; sleep -seconds 10; 101..200 } | less.exe
show output immediately.

BrucePay · 2018-03-14T21:44:29Z

src/System.Management.Automation/engine/InitialSessionState.cs

+
+    $process = [System.Diagnostics.Process]::new()
+
+    $process.StartInfo.FileName = 'C:\Windows\system32\more.com'


Probably should use $ENV:windir instead of hard coding "c:\windows".

iSazonov · 2018-03-15T14:16:13Z

@BrucePay Good catch! I think it is best way. Seems we can use an alias for more.com/less.

Set-Alias -Name tst -Value "more.com"`
& { 1..100; sleep -seconds 10; 101..200 } | tst

show output immediately.
If it's acceptable I'll close the PR and make a new.

daxian-dbw · 2018-03-15T16:15:38Z

Removing more function would be a breaking change in that the more function takes multiple file path strings separated by comma ,, while more.com and less don't.

PS:11> more.com F:\test.txt, F:\new.txt
Cannot access file F:\test.txt

Besides, the more function uses a pager if env:PAGER is set. ~~Not sure if this is practically used by anyone though.~~ Apparently this is practically being used :)

rkeithhill · 2018-03-15T16:52:02Z

I use Pager on Windows in order to use less. An alias for Pager would be OK as long as I can override it and set it to less.

iSazonov · 2018-03-16T03:25:08Z

We could create the alias at PowerShell startup and keep current platform specific logic - assign more.com on Windows, less on Unix or use env:PAGER.

daxian-dbw · 2018-03-19T16:30:27Z

Update the PR title to add [WIP].

rkeithhill · 2018-03-20T00:34:51Z

src/System.Management.Automation/engine/InitialSessionState.cs

+
+    $process = [System.Diagnostics.Process]::new()
+
+    $process.StartInfo.FileName = 'C:\Windows\system32\more.com'


Why use a path that is only valid on Windows and a binary more.com that is only valid on Windows? Also, why does the pipeline case ignore the PAGER utility specified in the $env:PAGER? I always use less, even on Windows. I need to be able specify less on Windows. If I was forced to use more.com I'd go batsh!t in short order. ;-)

iSazonov · 2018-03-30T13:28:12Z

@daxian-dbw @SteveL-MSFT
Could you please discuss the breaking change (implement alias for more with default more.com on Windows and less on Unix and given env: PAGER) on PowerShell Committee? It seems to be easier than creating auxiliary process with redirects.

SteveL-MSFT · 2018-04-01T07:41:53Z

@iSazonov can you be more specific on what you think needs to be discussed by @PowerShell/powershell-committee? Is it whether we should remove the more function altogether?

iSazonov · 2018-04-01T11:09:42Z

@SteveL-MSFT Yes, we could try to remove more function - but it is a breaking change as @daxian-dbw said above ( more.com F:\test.txt, F:\new.txt ).

SteveL-MSFT · 2018-04-25T22:23:53Z

@PowerShell/powershell-committee discussed this and agrees we should remove the more function and move the $env:PAGER capability into the help function to preserve that experience. This is a Breaking Change, but it seems that this function has outlived its usefulness and we should respect the pager command the user specified (less vs more, etc...)

iSazonov · 2018-04-26T05:10:39Z

Should we implement an alias for more with the $env:PAGER capability?

New-Alias -Name more -Value more.com # for Windows
New-Alias -Name more -Value less     # for Unix

SteveL-MSFT · 2018-04-26T13:29:32Z

@iSazonov the discussion in the committee is that more should just be more.com on Windows which seems to be working and not have issues any more based on the comments in the function (we tested against a nanoserver docker container). It seemed wrong that users would have to use more to get $env:PAGER as they should just invoke that specific pager directly. For tools that require automatic paging (like help), it makes sense to respect $env:PAGER (we couldn't think of any other case outside of help function which itself is a separate discussion if we should have that instead of just alias help directly to get-help and support paging...)

This reverts commit e6c11fd.

This reverts commit 83f241d.

This reverts commit 87c24d8.

…lp function

iSazonov · 2018-04-29T14:31:23Z

@daxian-dbw @SteveL-MSFT The PR is ready for review.

rkeithhill · 2018-04-30T23:32:42Z

src/System.Management.Automation/engine/InitialSessionState.cs

-        $help | more
+        # Respect PAGER, use more on Windows, and use less on Linux
+        if (Test-Path env:PAGER) {
+            $moreCommand = (Get-Command -CommandType Application $env:PAGER | Select-Object -First 1).Definition


This will lose the ability to specify arguments to the pager utility - $pager,$moreArgs = $env:PAGER -split '\s+' . That doesn't bother me personally but presumably somebody added these for a reason (an issue I suspect).

Yes, that was explicitly to fix #6142

Should just use the same code as from https://github.com/PowerShell/PowerShell/pull/6144/files

Thanks for the reminder!

Fixed.

Is it possible to add tests to exclude such regression?

rkeithhill · 2018-05-01T15:02:12Z

src/System.Management.Automation/engine/InitialSessionState.cs

+            $moreCommand = (Get-Command -CommandType Application less | Select-Object -First 1).Definition
+        }
+
+        $help | & $moreCommand $moreArgs $moreArgs


IIRC someone (@BrucePay) had initialized $moreArgs to an empty string above the preceding if/else statements to prevent a strict mode failure ($moreArgs not being defined when $env:PAGER is not defined).

What about

$pager,$moreArgs = $env:PAGER -split '\s+' if ($pager != $null) { $moreCommand = (Get-Command -CommandType Application $pager | Select-Object -First 1).Definition } elseif ($IsWindows) { $moreCommand = (Get-Command -CommandType Application more | Select-Object -First 1).Definition } else { $moreCommand = (Get-Command -CommandType Application less | Select-Object -First 1).Definition } $help | & $moreCommand $moreArgs

You have $moreArgs listed twice after the invocation of $moreCommand. There should only be one.

I updated my post.

Also, watch out for $pager != $null. That should be -ne. That said, this line:

$pager,$moreArgs = $env:PAGER -split '\s+'

Will assign an empty string, not $null, to $pager when $env:PAGER is not defined. So I think the test should be simply: if ($pager). And ideally, if get-command can't find the app referred to by $env:PAGER, it should revert back to the default for the OS. Otherwise, you're left with a $null in $moreCommand and help | & $null will fail.

Why are we doing Get-Command -CommandType Application ? We get the same error for badapp, & badapp and Get-Command -CommandType Application badapp.

$pager,$moreArgs = $env:PAGER -split '\s+' if ($pager) { $moreCommand = $pager } elseif ($IsWindows) { $moreCommand = "more.exe" } else { $moreCommand = "less" } $help | & $moreCommand $moreArgs

daxian-dbw · 2018-05-01T21:31:14Z

src/System.Management.Automation/engine/InitialSessionState.cs

+            $moreCommand = (Get-Command -CommandType Application less | Select-Object -First 1).Definition
+        }
+
+        $help | & $moreCommand $moreArgs $moreArgs


You have $moreArgs listed twice after the invocation of $moreCommand. There should only be one.

Not only in your post, that's also the case in your code change.

I had to do rebase but I wanted to keep the commit history. As a result lost the essence.

The code was refactored.

iSazonov · 2018-05-02T13:10:32Z

Add a test for Strict Mode from #6781.

daxian-dbw · 2018-05-03T17:32:34Z

test/powershell/engine/Help/HelpSystem.Tests.ps1

+            Help
+        }
+        # the help function renders the help content as text so just verify that there is content
+        $help | Should -Not -BeNullOrEmpty $true


$true is not needed. See #6781 (comment)

rkeithhill

LGTM

iSazonov · 2018-05-05T17:10:29Z

@rkeithhill Thank you that you caught the lost commits! My mistake was that I did not rebase the old PR.

[Feature] Add pipeline support in 'more' function

87c24d8

iSazonov assigned daxian-dbw Jan 29, 2018

iSazonov requested review from BrucePay, daxian-dbw and lzybkr as code owners January 29, 2018 10:59

iSazonov added 2 commits January 29, 2018 16:26

Pass non-string objects through formatting system

83f241d

Use formatting system

e6c11fd

iSazonov mentioned this pull request Feb 12, 2018

Unpredictable ordering of native command output when merging the error stream into the success output stream #5424

Closed

lzybkr removed their request for review March 14, 2018 16:26

daxian-dbw reviewed Mar 14, 2018

View reviewed changes

BrucePay reviewed Mar 14, 2018

View reviewed changes

daxian-dbw changed the title ~~[Feature] Add pipeline support in 'more' function~~ [WIP] Add pipeline support in 'more' function Mar 19, 2018

daxian-dbw removed their assignment Mar 19, 2018

rkeithhill reviewed Mar 20, 2018

View reviewed changes

SteveL-MSFT added the Review - Committee The PR/Issue needs a review from the PowerShell Committee label Apr 2, 2018

SteveL-MSFT added Committee-Reviewed PS-Committee has reviewed this and made a decision and removed Review - Committee The PR/Issue needs a review from the PowerShell Committee labels Apr 25, 2018

Revert "Use formatting system"

641c5a4

This reverts commit e6c11fd.

iSazonov added 4 commits April 27, 2018 16:20

Revert "Pass non-string objects through formatting system"

09a6bdf

This reverts commit 83f241d.

Revert "[Feature] Add pipeline support in 'more' function"

3cc2c84

This reverts commit 87c24d8.

Remove 'more' function and move the $env:PAGER capability into the he…

7a3b9f4

…lp function

Merge branch 'master' into more-support-pipeline

a9b72eb

iSazonov added the Breaking-Change breaking change that may affect users label Apr 27, 2018

iSazonov changed the title ~~[WIP] Add pipeline support in 'more' function~~ Add pipeline support in 'more' function Apr 27, 2018

[Feature] Remove 'more' alias test

b25acd9

iSazonov force-pushed the more-support-pipeline branch from dd46181 to b25acd9 Compare April 27, 2018 12:51

iSazonov changed the title ~~Add pipeline support in 'more' function~~ Remove 'more' function and move the $env:PAGER capability into the help function Apr 27, 2018

daxian-dbw mentioned this pull request Apr 30, 2018

Fixed uninitialized variable in the help function that was causing it to fail in strict mode #6781

Closed

11 tasks

rkeithhill reviewed Apr 30, 2018

View reviewed changes

Add support arguments in Pager

bdee0ec

SteveL-MSFT approved these changes May 1, 2018

View reviewed changes

iSazonov assigned daxian-dbw May 1, 2018

rkeithhill reviewed May 1, 2018

View reviewed changes

daxian-dbw reviewed May 1, 2018

View reviewed changes

iSazonov added 2 commits May 2, 2018 18:13

Refactor help function

a792bd4

Add test for Strict Mode

31ab358

iSazonov force-pushed the more-support-pipeline branch from fbc9dc2 to 31ab358 Compare May 2, 2018 13:14

daxian-dbw reviewed May 3, 2018

View reviewed changes

Fix typo

b6f60be

daxian-dbw approved these changes May 3, 2018

View reviewed changes

rkeithhill approved these changes May 3, 2018

View reviewed changes

daxian-dbw merged commit c1026b3 into PowerShell:master May 4, 2018

iSazonov deleted the more-support-pipeline branch May 5, 2018 17:10


		$process = [System.Diagnostics.Process]::new()

		$process.StartInfo.FileName = 'C:\Windows\system32\more.com'

Remove 'more' function and move the $env:PAGER capability into the help function #6059

Remove 'more' function and move the $env:PAGER capability into the help function #6059

Conversation

iSazonov commented Jan 29, 2018 • edited

PR Summary

PR Checklist

iSazonov commented Jan 30, 2018

Choose a reason for hiding this comment

BrucePay left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iSazonov commented Mar 15, 2018

daxian-dbw commented Mar 15, 2018 • edited

rkeithhill commented Mar 15, 2018

iSazonov commented Mar 16, 2018

daxian-dbw commented Mar 19, 2018

Choose a reason for hiding this comment

iSazonov commented Mar 30, 2018 • edited

SteveL-MSFT commented Apr 1, 2018

iSazonov commented Apr 1, 2018

SteveL-MSFT commented Apr 25, 2018

iSazonov commented Apr 26, 2018 • edited

SteveL-MSFT commented Apr 26, 2018

iSazonov commented Apr 29, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iSazonov May 1, 2018 • edited

Choose a reason for hiding this comment

rkeithhill May 1, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rkeithhill May 1, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iSazonov commented May 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rkeithhill left a comment

Choose a reason for hiding this comment

iSazonov commented May 5, 2018

iSazonov commented Jan 29, 2018 •

edited

daxian-dbw commented Mar 15, 2018 •

edited

iSazonov commented Mar 30, 2018 •

edited

iSazonov commented Apr 26, 2018 •

edited

iSazonov May 1, 2018 •

edited

rkeithhill May 1, 2018 •

edited

rkeithhill May 1, 2018 •

edited