don't insert linebreaks to output (except for tables) #5193

Merged
merged 4 commits into from Oct 30, 2017

Conversation

Projects
None yet
4 participants
Owner

SteveL-MSFT commented Oct 21, 2017

don't use current console window width to add linebreaks to output (including debug, error, and verbose)
explicitly change errorrecord formatter to not add linebreaks
set width to int.maxvalue if not specified, in some special cases if width is int.maxvalue, set to console.windowwidth
so that you don't get int.maxvalue padding (like tables)

Related: #3813

@SteveL-MSFT SteveL-MSFT requested a review from lzybkr Oct 21, 2017

SteveL-MSFT added some commits Oct 21, 2017

[feature]
don't use current console window width to add linebreaks to output (including debug, error, and verbose)
explicitly change errorrecord formatter to not add linebreaks
set width to int.maxvalue if not specified, in some special cases if width is int.maxvalue, set to console.windowwidth
so that you don't get int.maxvalue padding (like tables)
[feature]
wrap calls to get WindowWidth and default to 120 columns if it fails
@@ -921,6 +921,17 @@ internal override void Initialize()
}
int columnsOnTheScreen = this.InnerCommand._lo.ColumnNumber;
+ if (columnsOnTheScreen == int.MaxValue)
@iSazonov

iSazonov Oct 23, 2017

Collaborator

Could you please add comments about the special cases?

@SteveL-MSFT

SteveL-MSFT Oct 24, 2017

Owner

Yes, will add

+ pwsh -c "& { [Console]::Error.WriteLine('$longtext') }" 2>&1 > $testdrive\error.txt
+ $e = Get-Content -Path $testdrive\error.txt
+ $e.Count | Should BeExactly 1
+ $e | Should Match $longtext
@iSazonov

iSazonov Oct 23, 2017

Collaborator

Maybe use "BeExactly"? It seems better for testing a formatting.

@SteveL-MSFT

SteveL-MSFT Oct 23, 2017

Owner

The problem currently is that redirection writes with a BOM, so this would fail

@iSazonov

iSazonov Oct 23, 2017

Collaborator

We read with Get-Content -Path $testdrive\error.txt - I believe there is not BOM if we don't use -Byte.

@SteveL-MSFT

SteveL-MSFT Oct 24, 2017

Owner

Good point about Get-Content, I'll change it

+ $origDebugPref = $DebugPreference
+ $DebugPreference = "Continue"
+ try {
+ $out = Write-Debug $text *>&1
@iSazonov

iSazonov Oct 23, 2017

Collaborator

Maybe use exactly "2>&1"

@SteveL-MSFT

SteveL-MSFT Oct 23, 2017

Owner

Debug isn’t stream number 2 and it seems you can only redirect debug if you redirect all streams

@iSazonov

iSazonov Oct 23, 2017

Collaborator

It seems works:

PS> $a=Write-Verbose "ext " -Verbose 4>&1
PS> $a
VERBOSE: ext
PS> $b=Write-Debug "qwerty " -Debug 5>&1
Confirm
Continue with this operation?
[Y] Yes  [A] Yes to All  [H] Halt Command  [S] Suspend  [?] Help (default is "Y"): y
PS C:\Users\sie> $b

DEBUG: qwerty
@SteveL-MSFT

SteveL-MSFT Oct 24, 2017

Owner

Maybe I did something wrong when I tried it. Will update.

+ $DebugPreference = "Continue"
+ try {
+ $out = Write-Debug $text *>&1
+ $out | Should Match $text
@iSazonov

iSazonov Oct 23, 2017

Collaborator

Maybe use "$out | Should BeExactly $longtext"?

@SteveL-MSFT

SteveL-MSFT Oct 23, 2017

Owner

Same problem as above in that redirection currently adds a BOM

+ $DebugPreference = $origDebugPref
+ }
+ }
+}
@iSazonov

iSazonov Oct 23, 2017

Collaborator

Please add new line at EOF.

+ pwsh -c Write-Error -Message $longtext 2>&1 > $testdrive\error.txt
+ $e = Get-Content -Path $testdrive\error.txt
+ $e.Count | Should BeExactly 4
+ $e[0] | Should Match $longtext
@iSazonov

iSazonov Oct 23, 2017

Collaborator

Can we use more strong match pattern? I'm again worried about false positives.

@SteveL-MSFT

SteveL-MSFT Oct 23, 2017

Owner

Write-Error adds extra info prefixed to the text. The important bits for this test is the long text not getting choppedby a linebreak

@iSazonov

iSazonov Oct 24, 2017

Collaborator

I see - no false positives can be.

Closed.

+ $origVerbosePref = $VerbosePreference
+ $VerbosePreference = "continue"
+ try {
+ $out = Write-Verbose $text *>&1
@iSazonov

iSazonov Oct 23, 2017

Collaborator

The same "2>&1".

@SteveL-MSFT

SteveL-MSFT Oct 23, 2017

Owner

Verbose isn’t stream 2, I believe it’s 3, but redirection only worked when using all streams

+ $VerbosePreference = "continue"
+ try {
+ $out = Write-Verbose $text *>&1
+ $out | Should Match $text
@iSazonov

iSazonov Oct 23, 2017

Collaborator

The same about "BeExactly".

@SteveL-MSFT

SteveL-MSFT Oct 23, 2017

Owner

Same as above, BOM gets added

[feature]
address iSazonov's comments
@@ -1248,7 +1206,7 @@ public override void WriteDebugLine(string message)
else
{
// NTRAID#Windows OS Bugs-1061752-2004/12/15-sburns should read a skin setting here...
@iSazonov

iSazonov Oct 24, 2017

Collaborator

Is the comment still actual?

@SteveL-MSFT

SteveL-MSFT Oct 24, 2017

Owner

I believe the comment is saying that if we decide to support color themes (aka skins), this is one place to put the code for Write-Debug. Did a quick search for "read a skin" and found four places for Debug, Verbose, Warning, and ProgressPane.

@iSazonov

iSazonov Oct 25, 2017

Collaborator

Thanks for clarify.

Closed.

@@ -921,6 +921,18 @@ internal override void Initialize()
}
int columnsOnTheScreen = this.InnerCommand._lo.ColumnNumber;
+ // for tables, we want to make sure we don't end up adding padding to int.MaxValue and default to 120 columns instead
@iSazonov

iSazonov Oct 24, 2017

Collaborator

Can we rephrase it for easy reading?

And why 120? Can we use current windows width? Have we tests for this?

@SteveL-MSFT

SteveL-MSFT Oct 24, 2017

Owner

Let me try to add a bit more detail to make it easier to understand. The code tries to use window width, but if it can't be read (like implicit remoting where there's no console window), we have to default to something. 120 was the original default without my changes.

[feature]
expanded on comment for computing width for tables

lzybkr approved these changes Oct 27, 2017

I tried out these changes - it looks like a big improvement for Format-List and no change for Format-Table or Format-Wide and error records aren't wrapped, so that's great.

Maybe in some future work we can detect when the output is to a file and be smarter about Format-Table and Format-Wide.

Member

daxian-dbw commented Oct 30, 2017

Given that @iSazonov's comments have been addressed, I will merge this PR.

@daxian-dbw daxian-dbw merged commit a384c6e into PowerShell:master Oct 30, 2017

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla All CLA requirements met.
Details

@SteveL-MSFT SteveL-MSFT deleted the SteveL-MSFT:no-linebreak-output branch Nov 12, 2017

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