Add NoNewLine switch for Out-String cmdlet by raghav710 · Pull Request #5056 · PowerShell/PowerShell

raghav710 · 2017-10-08T06:33:28Z

Fix #3684.

First PS pull request {insert nervous emoji}
Mostly adapted from the -NoNewLine changes for Out-File

msftclas · 2017-10-08T06:33:36Z

All CLA requirements met.

iSazonov · 2017-10-08T07:16:15Z

...rosoft.PowerShell.Commands.Utility/commands/utility/FormatAndOutput/out-string/out-string.cs

Please remove extra line.

Done as suggested

iSazonov · 2017-10-08T07:17:58Z

...rosoft.PowerShell.Commands.Utility/commands/utility/FormatAndOutput/out-string/out-string.cs

Please add braces here.

Added braces as suggested

iSazonov · 2017-10-08T07:18:41Z

...rosoft.PowerShell.Commands.Utility/commands/utility/FormatAndOutput/out-string/out-string.cs

Please use one line formatting as above.

Thanks for pointing that out. Done!

iSazonov · 2017-10-08T07:19:59Z

...rosoft.PowerShell.Commands.Utility/commands/utility/FormatAndOutput/out-string/out-string.cs

Please put NoNewLine and Sream parameters in different parameter sets as discussed in the Issue.

Done. I have named the ParameterSets as NonStreamFormatting and StreamFormatting for want of a better name. Do suggest one if this is bad :)
I've also fixed-up the commit instead of adding a new one to keep it clean. Let me know if that is an inconvenience.

iSazonov · 2017-10-08T07:20:38Z

test/powershell/Modules/Microsoft.PowerShell.Utility/Out-String.Tests.ps1

Why we use space? I'd prefer to remove.

I was not sure what example to give and hence followed the one in
It "Should accumulate the strings and returns them as a single string"
Will remove the space as suggested

removed the space

markekraus

LGTM.

markekraus · 2017-10-08T19:08:02Z

We just need to make sure the documentation is updated to include the new parameter.

iSazonov · 2017-10-08T10:35:10Z

...rosoft.PowerShell.Commands.Utility/commands/utility/FormatAndOutput/out-string/out-string.cs

I'd prefer "NoNewlineFormatting".

Nice. changing it now

iSazonov · 2017-10-08T10:38:18Z

test/powershell/Modules/Microsoft.PowerShell.Utility/Out-String.Tests.ps1

Please add the test for default parameter set:
$testArray | Out-String | Should Be "ab"

If i am not wrong, this is the test for the default parameter set wherein newlines are added to the output. Is my understanding correct?

Thanks! Yes, correct.

Closed.

iSazonov · 2017-10-08T10:40:52Z

test/powershell/Modules/Microsoft.PowerShell.Utility/Out-String.Tests.ps1

I think we should a test that Stream and NoNewLine in different parameter sets:

{$testarray | Out-String -NoNewLine -Stream } | ShouldBeErrorId "....."

Sure. I was a bit unsure how to go about doing that. Thanks for helping out!

daxian-dbw · 2017-10-10T00:58:40Z

test/powershell/Modules/Microsoft.PowerShell.Utility/Out-String.Tests.ps1

Please replace $($nl) with ${nl}

Sure. Done :)

daxian-dbw

@raghav710 Thanks for your contribution! Hope to see you around more.

One side note: It's recommended to push new commits to address comments instead of squashing new fixes into the existing commit. In this way, the change history is preserved and it's very helpful for code reviews.

raghav710 · 2017-10-11T05:53:30Z

@daxian-dbw Thanks for the note. Will follow that henceforth during code reviews. As an aside, do you think it is good to squash the commits after approval as that would keep the history clean in case there were multiple code comments.
Heartfelt thanks to you and @mklement0 @iSazonov @SteveL-MSFT . The comments were quite helpful!

iSazonov · 2017-10-11T13:42:41Z

@raghav710 We want keep the history while we review. Then we usually Squash and Merge for simple changes. But if the code is very complex, tricky and add big feature we keep all commits.

iSazonov · 2017-10-11T13:47:31Z

@mklement0 We merged the PR but we can always bring Out-String and Out-File into line until RTM.

raghav710 · 2017-10-12T17:39:11Z

Yaay!

raghav710 requested review from JamesWTruher, PaulHigin and dantraMSFT as code owners October 8, 2017 06:33

raghav710 force-pushed the fix-issue-3684 branch from 2fbea3c to 38a2f00 Compare October 8, 2017 06:47

iSazonov reviewed Oct 8, 2017

View reviewed changes

iSazonov changed the title ~~Add NoNewLine switch for Out-String cmdlet (#3684)~~ Add NoNewLine switch for Out-String cmdlet Oct 8, 2017

raghav710 force-pushed the fix-issue-3684 branch 2 times, most recently from d3a59c3 to f17122a Compare October 8, 2017 14:59

markekraus approved these changes Oct 8, 2017

View reviewed changes

iSazonov reviewed Oct 8, 2017

View reviewed changes

iSazonov added the Documentation Needed in this repo Documentation is needed in this repo label Oct 8, 2017

raghav710 force-pushed the fix-issue-3684 branch from f17122a to cfd5ebd Compare October 9, 2017 17:08

daxian-dbw requested changes Oct 10, 2017

View reviewed changes

daxian-dbw assigned iSazonov Oct 10, 2017

Add NoNewLine switch for Out-String cmdlet (PowerShell#3684)

eb5cb2a

raghav710 force-pushed the fix-issue-3684 branch from cfd5ebd to eb5cb2a Compare October 10, 2017 06:34

daxian-dbw approved these changes Oct 10, 2017

View reviewed changes

iSazonov merged commit 7dd36c9 into PowerShell:master Oct 11, 2017

joeyaiello mentioned this pull request Oct 15, 2018

Docs needed for 'Add NoNewLine switch for Out-String cmdlet' MicrosoftDocs/PowerShell-Docs#3077

Closed

joeyaiello removed Documentation Needed in this repo Documentation is needed in this repo labels Oct 15, 2018

Conversation

raghav710 commented Oct 8, 2017 • edited by iSazonov Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

msftclas commented Oct 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

markekraus left a comment

Choose a reason for hiding this comment

Uh oh!

markekraus commented Oct 8, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daxian-dbw left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

raghav710 commented Oct 11, 2017

Uh oh!

iSazonov commented Oct 11, 2017

Uh oh!

iSazonov commented Oct 11, 2017

Uh oh!

raghav710 commented Oct 12, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

raghav710 commented Oct 8, 2017 •

edited by iSazonov

Loading

msftclas commented Oct 8, 2017 •

edited

Loading

daxian-dbw left a comment •

edited

Loading