Skip to content

Conversation

SteveL-MSFT
Copy link
Member

@SteveL-MSFT SteveL-MSFT commented Nov 13, 2018

PR Summary

When formatting, the Group start and end added unnecessary newlines which resulted in double newlines in many cases. Formatting changes are not considered breaking.

(get-module | out-string).replace("`n","\n`n")

Before:

\n
ModuleType Version    Name                                ExportedCommands\n
---------- -------    ----                                ----------------\n
Manifest   6.1.0.0    Microsoft.PowerShell.Management     {Add-Content, Clear-Content, Clear-Item, Clear-ItemProperty...}\n
Manifest   6.1.0.0    Microsoft.PowerShell.Utility        {Add-Member, Add-Type, Clear-Variable, Compare-Object...}\n
Script     2.0.0      PSReadLine                          {Get-PSReadLineKeyHandler, Get-PSReadLineOption, Remove-PSReadLineKeyHandler, Set-PSReadLineKeyHandler...}\n
\n
\n

After:

\n             
ModuleType Version    Name                                ExportedCommands\n
---------- -------    ----                                ----------------\n
Manifest   6.1.0.0    Microsoft.PowerShell.Management     {Add-Content, Clear-Content, Clear-Item, Clear-ItemProperty...}\n
Manifest   6.1.0.0    Microsoft.PowerShell.Utility        {Add-Member, Add-Type, Clear-Variable, Compare-Object...}\n
Script     2.0.0      PSReadLine                          {Get-PSReadLineKeyHandler, Get-PSReadLineOption, Remove-PSReadLineKeyHandler, Set-PSReadLineKeyHandler...}\n
\n

There are still a few cases where the final output has two newlines (but not three anymore!). This is a bit more complicated to fix and would need to track state that either a newline was the last thing output or the item is the last in a collection and doesn't need a separator.

Fix #7186

PR Checklist

remove extra whitespace that is unnecessary and results in double newlines
@iSazonov
Copy link
Collaborator

What we get with:

dir | Format-Table -GroupBy Name

@SteveL-MSFT
Copy link
Member Author

SteveL-MSFT commented Nov 13, 2018

(get-process pwsh | format-list | Out-String).Replace("`n","\n`n")

Before:

\n
\n
Id      : 1626\n
Handles : 0\n
CPU     : 5697.7865177\n
SI      : 422\n
Name    : pwsh\n
\n
Id      : 18927\n
Handles : 0\n
CPU     : 480.0566228\n
SI      : 18920\n
Name    : pwsh\n
\n
Id      : 18928\n
Handles : 0\n
CPU     : 771.2722798\n
SI      : 422\n
Name    : pwsh\n
\n
\n
\n

After:

\n
Id      : 1626\n
Handles : 0\n
CPU     : 5696.2817117\n
SI      : 422\n
Name    : pwsh\n
\n
Id      : 18927\n
Handles : 0\n
CPU     : 474.75417\n
SI      : 18920\n
Name    : pwsh\n
\n
Id      : 18928\n
Handles : 0\n
CPU     : 769.8305049\n
SI      : 422\n
Name    : pwsh\n
\n
\n

@SteveL-MSFT
Copy link
Member Author

SteveL-MSFT commented Nov 13, 2018

(dir | select -first 3 |Format-Table -GroupBy Name | out-string).replace("`n","\n`n")

Before:

\n
\n
    Directory: /Users/steve/repos/PowerShell\n
\n
\n
Mode                LastWriteTime         Length Name\n
----                -------------         ------ ----\n
d-----           11/4/18  7:47 AM                assets\n
\n
\n
    Directory: /Users/steve/repos/PowerShell\n
\n
\n
Mode                LastWriteTime         Length Name\n
----                -------------         ------ ----\n
d-----          10/26/18 10:24 AM                demos\n
\n
\n
    Directory: /Users/steve/repos/PowerShell\n
\n
\n
Mode                LastWriteTime         Length Name\n
----                -------------         ------ ----\n
d-----          10/26/18 10:24 AM                docker\n
\n
\n

After:

\n
\n
    Directory: /Users/steve/repos/PowerShell\n
\n
Mode                LastWriteTime         Length Name\n
----                -------------         ------ ----\n
d-----           11/4/18  7:47 AM                assets\n
\n
    Directory: /Users/steve/repos/PowerShell\n
\n
Mode                LastWriteTime         Length Name\n
----                -------------         ------ ----\n
d-----          10/26/18 10:24 AM                demos\n
\n
    Directory: /Users/steve/repos/PowerShell\n
\n
Mode                LastWriteTime         Length Name\n
----                -------------         ------ ----\n
d-----          10/26/18 10:24 AM                docker\n
\n

@iSazonov
Copy link
Collaborator

Is it ok to have one '\n' before "Directory ..."?

@SteveL-MSFT
Copy link
Member Author

@iSazonov you are asking if it's ok to separate the groups with just a single newline vs two? Seems fine to me and just as readable visually. In the future, we should leverage vt100 as a way to provide visual indicators rather than rely on whitespace only.

Test-ValidateSet 'Hello' | Should -BeExactly 'Hello'
} finally {
Remove-Module -Name $moduleFile -Force
Remove-Module -Name $moduleFile -Force -ErrorAction SilentlyContinue

Choose a reason for hiding this comment

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

This is not related to this formatting code change, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Correct. Was running the tests locally and this line was outputting unncessarily

@iSazonov iSazonov merged commit 43e0394 into PowerShell:master Nov 14, 2018
@SteveL-MSFT SteveL-MSFT deleted the format-whitespace branch November 14, 2018 06:50
@adityapatwardhan adityapatwardhan added the CL-Engine Indicates that a PR should be marked as an engine change in the Change Log label Nov 15, 2018
@adityapatwardhan
Copy link
Member

@iSazonov Reminder to add CL-XXX labels.

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

Labels

CL-Engine Indicates that a PR should be marked as an engine change in the Change Log

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Remove unnecessary newlines from output

4 participants