8000 Update `Clear-Host` to simply call `[Console]::Clear()` on all platforms and remove `clear` alias on Unix systems by SteveL-MSFT · Pull Request #8603 · PowerShell/PowerShell · GitHub
[go: up one dir, main page]
Skip to content

Update Clear-Host to simply call [Console]::Clear() on all platforms and remove clear alias on Unix systems #8603

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
Merged
iSazonov merged 1 commit into from
Jan 8, 2019
Merged

Update Clear-Host to simply call [Console]::Clear() on all platforms and remove clear alias on Unix systems #8603

iSazonov merged 1 commit into from
Jan 8, 2019

Conversation

@SteveL-MSFT
Copy link
Member
@SteveL-MSFT SteveL-MSFT commented Jan 7, 2019 •
edited
Loading

PR Summary

Based on @PowerShell/powershell-committee decision, standardizing on [Console]::Clear() on all platforms and removing clear alias on non-Windows.

Fix #8554

PR Checklist

daxian-dbw
daxian-dbw approved these changes Jan 7, 2019
View reviewed changes
Copy link
Member
@daxian-dbw daxian-dbw left a comment

Choose a reason for hiding this comment

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

LGTM

@iSazonov
Copy link
Collaborator
iSazonov commented Jan 8, 2019

removing clear alias on non-Windows

@SteveL-MSFT Please reword the PR description. We are not "removing on non-Windows" but "restoring on Windows".

@mklement0
Copy link
Contributor
mklement0 commented Jan 8, 2019

@SteveL-MSFT, while we're at it, perhaps we can make the function an advanced one so that it doesn't quietly accept and ignore arbitrary arguments (even though none of the common parameters make sense).

E.g, the following is currently quietly accepted:

Clear-Host -NoSuchParam

@mklement0 mklement0 mentioned this pull request Jan 8, 2019
Closed
@iSazonov
Copy link
Collaborator
iSazonov commented Jan 8, 2019

@mklement0 What benefits we get?

@mklement0
Copy link
Contributor
mklement0 commented Jan 8, 2019 •
edited
Loading

@iSazonov:

The benefit is an abstract, general one, for consistency and predictability:

Don't make any command accept parameters that it is not designed to accept; in particular, don't make it quietly ignore such parameters.

@vexx32
Copy link
Collaborator
vexx32 commented Jan 8, 2019

In general I would think it best practice for any "official" functions be thoroughly implemented, as @mklement0 says. It sets a bad example if we go around including low-quality code in official distributions, and it goes against years of best practices to widely distribute a function that quietly ignores arbitrary parameters.

@SteveL-MSFT
Copy link
Member Author
SteveL-MSFT commented Jan 8, 2019

@iSazonov not sure what you mean by "restoring on Windows" since prior to this PR, the clear alias is on Windows, Linux, and macOS. This PR retains clear alias on Windows and removes it from Linux and macOS.

@mklement0 personally, I think we can consider extending Clear-Host to have a -IncludeScrollbackBuffer type of switch, but that changing it to an advanced function is outside the scope of this PR. I would suggest opening a new issue. If we go that route, I would suggest having it as an actual C# cmdlet which can be extended.

@iSazonov
Copy link
Collaborator
iSazonov commented Jan 8, 2019

@SteveL-MSFT Sorry, I lost the context.

@iSazonov iSazonov added Committee-Reviewed PS-Committee has reviewed this and made a decision CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log labels Jan 8, 2019
@iSazonov iSazonov merged commit 031cbf0 into PowerShell:master Jan 8, 2019
@SteveL-MSFT SteveL-MSFT deleted the clear-host-alias branch January 8, 2019 18:30
BrucePay
BrucePay reviewed Jan 8, 2019
View reviewed changes
src/System.Management.Automation/engine/InitialSessionState.cs
& (Get-Command -CommandType Application clear | Select-Object -First 1).Definition
return @"
[Console]::Clear()
# .Link
Copy link
Collaborator
@BrucePay BrucePay Jan 8, 2019

Choose a reason for hiding this comment

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

This isn't going to work for non-console hosts like ISE, VSCode(?) and remoting. On those platforms, you still need to use the $RawUI portability APIs. A nice solution would be to actually add a new RawUI.Clear() API and then implement that using Console.Clear() in the console host but that may end up being a bunch of work...

@SteveL-MSFT SteveL-MSFT mentioned this pull request Jan 9, 2019
Merged
11 tasks
@HumanEquivalentUnit HumanEquivalentUnit mentioned this pull request Jul 28, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@daxian-dbw daxian-dbw daxian-dbw approved these changes

@iSazonov iSazonov iSazonov approved these changes

@anmenaga anmenaga Awaiting requested review from anmenaga

+1 more reviewer

@BrucePay BrucePay BrucePay left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@iSazonov iSazonov

Labels

CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log Committee-Reviewed PS-Committee has reviewed this and made a decision

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

'clear' should leverage tput reset on *nix

6 participants

@SteveL-MSFT @iSazonov @mklement0 @vexx32 @daxian-dbw @BrucePay
0