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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 13 commits into from May 4, 2018
Merged

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

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
87c24d8
[Feature] Add pipeline support in 'more' function
iSazonov Jan 29, 2018
83f241d
Pass non-string objects through formatting system
iSazonov Jan 29, 2018
e6c11fd
Use formatting system
iSazonov Jan 30, 2018
641c5a4
Revert "Use formatting system"
iSazonov Apr 27, 2018
09a6bdf
Revert "Pass non-string objects through formatting system"
iSazonov Apr 27, 2018
3cc2c84
Revert "[Feature] Add pipeline support in 'more' function"
iSazonov Apr 27, 2018
7a3b9f4
Remove 'more' function and move the $env:PAGER capability into the he…
iSazonov Apr 27, 2018
a9b72eb
Merge branch 'master' into more-support-pipeline
iSazonov Apr 27, 2018
b25acd9
[Feature] Remove 'more' alias test
iSazonov Apr 27, 2018
bdee0ec
Add support arguments in Pager
iSazonov May 1, 2018
a792bd4
Refactor help function
iSazonov May 2, 2018
31ab358
Add test for Strict Mode
iSazonov May 2, 2018
b6f60be
Fix typo
iSazonov May 3, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
48 changes: 17 additions & 31 deletions src/System.Management.Automation/engine/InitialSessionState.cs
View file
Open in desktop
Expand Up @@ -4250,8 +4250,12 @@ .FORWARDHELPCATEGORY Cmdlet
$PSBoundParameters['Full'] = $true
}

# Set the outputencoding to Console::OutputEncoding. More.com doesn't work well with Unicode.
$outputEncoding=[System.Console]::OutputEncoding
# Nano needs to use Unicode, but Windows and Linux need the default
$OutputEncoding = if ([System.Management.Automation.Platform]::IsNanoServer -or [System.Management.Automation.Platform]::IsIoT) {
[System.Text.Encoding]::Unicode
} else {
[System.Console]::OutputEncoding
}

$help = Get-Help @PSBoundParameters

Expand All @@ -4262,7 +4266,17 @@ .FORWARDHELPCATEGORY Cmdlet
}
else
{
$help | more
# Respect PAGER, use more on Windows, and use less on Linux
if (Test-Path env:PAGER) {
$pager,$moreArgs = $env:PAGER -split '\s+'
$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 $moreArgs
Copy link
Collaborator

Choose a reason for hiding this comment

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

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).

Copy link
Member

Choose a reason for hiding this comment

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

#6781

Copy link
Collaborator Author

@iSazonov iSazonov May 1, 2018 •
edited

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

@rkeithhill rkeithhill May 1, 2018 •
edited

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I updated my post.

Copy link
Collaborator

@rkeithhill rkeithhill May 1, 2018 •
edited

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

The code was refactored.

}
";
}
Expand Down Expand Up @@ -4743,32 +4757,6 @@ internal static SessionStateAliasEntry[] BuiltInAliases
# .Link
# https://go.microsoft.com/fwlink/?LinkID=225750
# .ExternalHelp System.Management.Automation.dll-help.xml
";

internal const string DefaultMoreFunctionText = @"
param([string[]]$paths)
# Nano needs to use Unicode, but Windows and Linux need the default
$OutputEncoding = if ([System.Management.Automation.Platform]::IsNanoServer -or [System.Management.Automation.Platform]::IsIoT) {
[System.Text.Encoding]::Unicode
} else {
[System.Console]::OutputEncoding
}

# Respect PAGER, use more on Windows, and use less on Linux
if (Test-Path env:PAGER) {
$pager,$moreArgs = $env:PAGER -split '\s+'
$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
}

if($paths) {
foreach ($file in $paths) {
Get-Content $file | & $moreCommand $moreArgs
}
} else { $input | & $moreCommand $moreArgs }
";

internal const string DefaultSetDriveFunctionText = "Set-Location $MyInvocation.MyCommand.Name";
Expand All @@ -4780,8 +4768,6 @@ internal static SessionStateAliasEntry[] BuiltInAliases
SessionStateFunctionEntry.GetDelayParsedFunctionEntry("prompt", DefaultPromptFunctionText, isProductCode: true),
SessionStateFunctionEntry.GetDelayParsedFunctionEntry("TabExpansion2", s_tabExpansionFunctionText, isProductCode: true),
SessionStateFunctionEntry.GetDelayParsedFunctionEntry("Clear-Host", GetClearHostFunctionText(), isProductCode: true),
// Porting note: we keep more because the function acts correctly on Linux
SessionStateFunctionEntry.GetDelayParsedFunctionEntry("more", DefaultMoreFunctionText, isProductCode: true),
SessionStateFunctionEntry.GetDelayParsedFunctionEntry("help", GetHelpPagingFunctionText(), isProductCode: true),
// Porting note: we remove mkdir on Linux because it is a conflict
#if !UNIX
Expand Down
4 changes: 0 additions & 4 deletions test/powershell/engine/Basic/DefaultCommands.Tests.ps1
View file
Open in desktop
Expand Up @@ -529,8 +529,4 @@ Describe "Verify approved aliases list" -Tags "CI" {
$result | Write-Host
$result | Should -BeNullOrEmpty
}

It "Should have 'more' as a function" {
Test-Path Function:more | Should -BeTrue
}
}