Skip to content

Remove extra newlines from formatting which resulted in necessary double newlines#8247

Merged
iSazonov merged 1 commit intoPowerShell:masterfrom
SteveL-MSFT:format-whitespace
Nov 14, 2018
Merged

Remove extra newlines from formatting which resulted in necessary double newlines#8247
iSazonov merged 1 commit intoPowerShell:masterfrom
SteveL-MSFT:format-whitespace

Conversation

@SteveL-MSFT
Copy link
Member

@SteveL-MSFT SteveL-MSFT commented Nov 13, 2018 •
edited
Loading

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

@SteveL-MSFT
[feature]
49f7c17 
remove extra whitespace that is unnecessary and results in double newlines
@iSazonov
Copy link
Collaborator

iSazonov commented Nov 13, 2018

What we get with:

dir | Format-Table -GroupBy Name

@SteveL-MSFT
Copy link
Member Author

SteveL-MSFT commented Nov 13, 2018 •
edited
Loading 

(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 •
edited
Loading 

(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

iSazonov commented Nov 13, 2018

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

@SteveL-MSFT
Copy link
Member Author

SteveL-MSFT commented Nov 13, 2018

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

anmenaga
anmenaga approved these changes Nov 13, 2018
View reviewed changes
test/powershell/Language/Classes/Scripting.Classes.Attributes.Tests.ps1
Test-ValidateSet 'Hello' | Should -BeExactly 'Hello'
} finally {
Remove-Module -Name $moduleFile -Force
Remove-Module -Name $moduleFile -Force -ErrorAction SilentlyContinue
Copy link

@anmenaga anmenaga Nov 13, 2018

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

@SteveL-MSFT SteveL-MSFT Nov 13, 2018

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

adityapatwardhan commented Nov 15, 2018

@iSazonov Reminder to add CL-XXX labels.

@iSazonov iSazonov mentioned this pull request Dec 28, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@iSazonov iSazonov iSazonov approved these changes

+1 more reviewer

@anmenaga anmenaga anmenaga approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@iSazonov iSazonov

Labels

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

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Remove unnecessary newlines from output

4 participants

@SteveL-MSFT @iSazonov @adityapatwardhan @anmenaga