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

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

Merged
merged 1 commit into from Jan 8, 2019

Conversation

Projects
None yet
6 participants
@SteveL-MSFT
Copy link
Member

SteveL-MSFT commented Jan 7, 2019

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

@SteveL-MSFT SteveL-MSFT force-pushed the SteveL-MSFT:clear-host-alias branch from e878053 to 8e5e5fe Jan 7, 2019

@daxian-dbw
Copy link
Member

daxian-dbw left a comment

LGTM

@iSazonov

This comment has been minimized.

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

This comment has been minimized.

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
@iSazonov

This comment has been minimized.

Copy link
Collaborator

iSazonov commented Jan 8, 2019

@mklement0 What benefits we get?

@mklement0

This comment has been minimized.

Copy link
Contributor

mklement0 commented Jan 8, 2019

@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

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Collaborator

iSazonov commented Jan 8, 2019

@SteveL-MSFT Sorry, I lost the context.

@iSazonov iSazonov merged commit 031cbf0 into PowerShell:master Jan 8, 2019

6 checks passed

CodeFactor 1 issue fixed.
Details
PowerShell-CI-linux #PR-8603-20190107.02 succeeded
Details
PowerShell-CI-macos #PR-8603-20190107.02 succeeded
Details
PowerShell-CI-static-analysis #PR-8603-20190107.01 succeeded
Details
PowerShell-CI-windows #PR-8603-20190107.02 succeeded
Details
license/cla All CLA requirements met.
Details

@SteveL-MSFT SteveL-MSFT deleted the SteveL-MSFT:clear-host-alias branch Jan 8, 2019

return @"
& (Get-Command -CommandType Application clear | Select-Object -First 1).Definition
return @"
[Console]::Clear()
# .Link

This comment has been minimized.

@BrucePay

BrucePay Jan 8, 2019

Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment