Skip to content

Format-Table does not return null if the Console width is equal to 0. #6883

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
Closed
kalgiz wants to merge 12 commits into from
Closed

Format-Table does not return null if the Console width is equal to 0. #6883

kalgiz wants to merge 12 commits into from

Conversation

@kalgiz
Copy link
Contributor

@kalgiz kalgiz commented May 16, 2018 •
edited by TravisEz13
Loading

PR Summary

Format-Table does not return null if the Console width is equal to 0.
Fixes: #6748

PR Checklist

This change is Reviewable

@kalgiz kalgiz force-pushed the format-table-bug-fix branch from a8afc94 to 948b47c Compare May 16, 2018 21:55
@kalgiz kalgiz mentioned this pull request May 16, 2018
Closed
11 tasks
@kalgiz kalgiz requested a review from JamesWTruher May 16, 2018 23:04
@daxian-dbw daxian-dbw requested review from SteveL-MSFT and removed request for daxian-dbw May 17, 2018 00:45
iSazonov
iSazonov reviewed May 17, 2018
View reviewed changes
test/powershell/Modules/Microsoft.PowerShell.Utility/Format-Table.Tests.ps1 Outdated
Copy link
Collaborator

@iSazonov iSazonov May 17, 2018

Choose a reason for hiding this comment

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

I think we should use try {} finally {} to ensure that the hook was turned off.

Copy link
Contributor Author

@kalgiz kalgiz May 17, 2018

Choose a reason for hiding this comment

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

Fixed.

test/powershell/Modules/Microsoft.PowerShell.Utility/Format-Table.Tests.ps1 Outdated
Copy link
Collaborator

@iSazonov iSazonov May 17, 2018

Choose a reason for hiding this comment

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

Should we check that console width is default (120)?

Copy link
Contributor Author

@kalgiz kalgiz May 17, 2018

Choose a reason for hiding this comment

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

Fixed.

src/System.Management.Automation/FormatAndOutput/common/BaseOutputtingCommand.cs Outdated
Copy link
Collaborator

@iSazonov iSazonov May 17, 2018

Choose a reason for hiding this comment

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

Maybe add in the comment that VSTS console width is 0 and we address this.

@iSazonov iSazonov requested a review from daxian-dbw May 17, 2018 05:51
@iSazonov iSazonov self-assigned this May 17, 2018
@kalgiz kalgiz force-pushed the format-table-bug-fix branch from 948b47c to b741b1e Compare May 17, 2018 22:19
iSazonov
iSazonov reviewed May 18, 2018
View reviewed changes
src/System.Management.Automation/FormatAndOutput/common/BaseOutputtingCommand.cs Outdated
Copy link
Collaborator

@iSazonov iSazonov May 18, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

The test (console width is default (120)) failed and this say the we have broken hook - Console.WindowWidth = 0 doesn't work.
The hook should be here:

if (Console.WindowWidth == 0 || InternalTestHooks.SetConsoleWidthToZero)

Copy link
Contributor Author

@kalgiz kalgiz May 18, 2018

Choose a reason for hiding this comment

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

Fixed.

test/powershell/Modules/Microsoft.PowerShell.Utility/Format-Table.Tests.ps1 Outdated
Copy link
Collaborator

@iSazonov iSazonov May 18, 2018

Choose a reason for hiding this comment

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

The test is failed. It is incorrect test. Console.WindowWidth = 0 doesn't work.
We should have another way.

Copy link
Collaborator

@iSazonov iSazonov May 22, 2018

Choose a reason for hiding this comment

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

@kalgiz Please address the comment.

iSazonov
iSazonov reviewed May 29, 2018
View reviewed changes
test/powershell/Modules/Microsoft.PowerShell.Utility/Format-Table.Tests.ps1 Outdated
Copy link
Collaborator

@iSazonov iSazonov May 29, 2018

Choose a reason for hiding this comment

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

Please replace aliases and correct formatting.

Copy link
Contributor Author

@kalgiz kalgiz Jun 4, 2018

Choose a reason for hiding this comment

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

Fixed.

iSazonov
iSazonov reviewed May 29, 2018
View reviewed changes
src/System.Management.Automation/FormatAndOutput/common/BaseOutputtingCommand.cs Outdated
Copy link
Collaborator

@iSazonov iSazonov May 29, 2018

Choose a reason for hiding this comment

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

Please add new line.

Copy link
Contributor Author

@kalgiz kalgiz Jun 4, 2018

Choose a reason for hiding this comment

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

Fixed.

@iSazonov
Copy link
Collaborator

iSazonov commented Jun 5, 2018

@kalgiz The PR contains alien commits - please rebase.

@kalgiz kalgiz force-pushed the format-table-bug-fix branch from 9465770 to 012739f Compare June 8, 2018 18:43
@kalgiz kalgiz force-pushed the format-table-bug-fix branch from 012739f to 902d657 Compare June 8, 2018 18:47
@kalgiz
Copy link
Contributor Author

kalgiz commented Jun 8, 2018

@iSazonov I made the rebase.

@iSazonov
Copy link
Collaborator

iSazonov commented Jun 9, 2018

@SteveL-MSFT @adityapatwardhan Could you please review?

adityapatwardhan
adityapatwardhan requested changes Jun 12, 2018
View reviewed changes
test/powershell/Modules/Microsoft.PowerShell.Utility/Format-Table.Tests.ps1
# Fill the console window with the string, so that it reaches its max width.
# Check if the max width is equal to default value (120), to test test hook set.
$testObject = @{ test = '1' * 200}
Format-Table -inputobject $testObject | Out-String -str | ForEach-Object {$_.length} | Sort-Object | Select-Object -Last 1 | Should -Be 120
Copy link
Member

@adityapatwardhan adityapatwardhan Jun 12, 2018

Choose a reason for hiding this comment

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

-str should be -Stream

Copy link
Member

@TravisEz13 TravisEz13 Jun 12, 2018

Choose a reason for hiding this comment

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

@kalgiz Can you make these changes?

Copy link
Member

@SteveL-MSFT SteveL-MSFT Jul 12, 2018

Choose a reason for hiding this comment

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

@adityapatwardhan can you make that change in this PR so we can still take this?

Copy link
Member

@TravisEz13 TravisEz13 Jul 16, 2018

Choose a reason for hiding this comment

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

@adityapatwardhan

@stale
Copy link

stale bot commented Jul 12, 2018

This PR has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed if no further activity occurs within 10 days.
Thank you for your contributions.
Community members are welcome to grab these works.

@stale stale bot added Stale and removed Stale labels Jul 12, 2018
@iSazonov iSazonov mentioned this pull request Aug 7, 2018
Merged
11 tasks
TravisEz13 added a commit to TravisEz13/PowerShell that referenced this pull request Aug 7, 2018
@TravisEz13
Port changes from PowerShell#6883 by @kalgiz
8143c43
TravisEz13 added a commit to TravisEz13/PowerShell that referenced this pull request Aug 7, 2018
@TravisEz13
Address PR comments from PowerShell#6883
587d60c
@TravisEz13
Copy link
Member

TravisEz13 commented Aug 7, 2018

replaced by #7465

@TravisEz13 TravisEz13 closed this Aug 7, 2018
TravisEz13 added a commit that referenced this pull request Aug 7, 2018
@TravisEz13
Default to DefaultConsoleWidth when DotNet says WindowWidth is 0 (#7465)
d29349b 
Default to DefaultConsoleWidth when DotNet says WindowWidth is 0

This resolves an issue in an environment like VSTS when pwsh is spawned and DotNet is not able to determine the Console Width.
* Port changes from #6883 by @kalgiz
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@TravisEz13 TravisEz13 TravisEz13 approved these changes

@adityapatwardhan adityapatwardhan adityapatwardhan requested changes

@iSazonov iSazonov iSazonov approved these changes

@BrucePay BrucePay Awaiting requested review from BrucePay

@JamesWTruher JamesWTruher Awaiting requested review from JamesWTruher

@SteveL-MSFT SteveL-MSFT Awaiting requested review from SteveL-MSFT

@daxian-dbw daxian-dbw Awaiting requested review from daxian-dbw

@anmenaga anmenaga Awaiting requested review from anmenaga

@joeyaiello joeyaiello Awaiting requested review from joeyaiello

Assignees

@iSazonov iSazonov

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Format-Table doesn't work when run on vsts.

5 participants

@kalgiz @iSazonov @TravisEz13 @SteveL-MSFT @adityapatwardhan