Add NoNewLine switch for Out-String cmdlet #5056

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

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

+                    _buffer.AppendLine(s);
+                }
+            }
+


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

@@ -124,7 +143,17 @@ private void OnWriteLine(string s)
            if (_stream)
                this.WriteObject(s);
            else


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

+            set
+            {
+                _suppressNewline = value;
+            }


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

+        /// False to add a newline to the end of the output string, true if not.
+        /// </summary>
+        [Parameter]
+        public SwitchParameter NoNewline


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

@@ -45,4 +45,14 @@ Describe "Out-String" -Tags "CI" {

 	$streamoutputlength | Should BeLessThan $nonstreamoutputlength
    }
+
+    It "Should not print a newline when the nonewline switch is used" {
+        $testArray = "a", " b"


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

+        /// <summary>
+        /// False to add a newline to the end of the output string, true if not.
+        /// </summary>
+        [Parameter(ParameterSetName="NonStreamFormatting")]


I'd prefer "NoNewlineFormatting".

Nice. changing it now

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

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

+
+    It "Should not print a newline when the nonewline switch is used" {
+        $testArray = "a", "b"
+        $testArray | Out-String -NoNewLine | Should Be "ab"


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

+    It "Should preserve embedded newline when the nonewline switch is used" {
+        $testArray = "a$nl", "b"
+        $testArray | Out-String -NoNewLine | Should Be "a$($nl)b"
+    }
 }


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

+
+    It "Should preserve embedded newline when the nonewline switch is used" {
+        $testArray = "a$nl", "b"
+        $testArray | Out-String -NoNewLine | Should Be "a$($nl)b"


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 dantraMSFT, JamesWTruher and PaulHigin 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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add NoNewLine switch for Out-String cmdlet #5056

Add NoNewLine switch for Out-String cmdlet #5056

raghav710 commented Oct 8, 2017 •

edited by iSazonov

msftclas commented Oct 8, 2017 •

edited

iSazonov Oct 8, 2017

raghav710 Oct 8, 2017

iSazonov Oct 8, 2017

raghav710 Oct 8, 2017

iSazonov Oct 8, 2017

raghav710 Oct 8, 2017

iSazonov Oct 8, 2017

raghav710 Oct 8, 2017

iSazonov Oct 8, 2017

raghav710 Oct 8, 2017

raghav710 Oct 8, 2017

markekraus left a comment

markekraus commented Oct 8, 2017

iSazonov Oct 8, 2017

raghav710 Oct 9, 2017

iSazonov Oct 8, 2017

raghav710 Oct 9, 2017

iSazonov Oct 9, 2017

iSazonov Oct 8, 2017

raghav710 Oct 9, 2017

daxian-dbw Oct 10, 2017

raghav710 Oct 10, 2017

daxian-dbw left a comment •

edited

raghav710 commented Oct 11, 2017

iSazonov commented Oct 11, 2017

iSazonov commented Oct 11, 2017

raghav710 commented Oct 12, 2017

Add NoNewLine switch for Out-String cmdlet #5056

Add NoNewLine switch for Out-String cmdlet #5056

Conversation

raghav710 commented Oct 8, 2017 • edited by iSazonov

msftclas commented Oct 8, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markekraus left a comment

Choose a reason for hiding this comment

markekraus commented Oct 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daxian-dbw left a comment • edited

Choose a reason for hiding this comment

raghav710 commented Oct 11, 2017

iSazonov commented Oct 11, 2017

iSazonov commented Oct 11, 2017

raghav710 commented Oct 12, 2017

raghav710 commented Oct 8, 2017 •

edited by iSazonov

msftclas commented Oct 8, 2017 •

edited

daxian-dbw left a comment •

edited