GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — Add NoNewLine switch for Out-String cmdlet by iamraghavs · Pull Request #5056 · PowerShell/PowerShell

Conversation

Raghav S (iamraghavs) commented Oct 8, 2017 •

edited by iSazonov

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Contributor

Fix #3684.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Raghav S (iamraghavs) requested review from James Truher (JamesWTruher), Paul Higinbotham (PaulHigin) and Dan Travison (dantraMSFT) as code owners

October 8, 2017 06:33

Microsoft Contribution License Agreements (msftclas) commented Oct 8, 2017 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

All CLA requirements met.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Raghav S (iamraghavs) force-pushed the fix-issue-3684 branch from 2fbea3c to 38a2f00 Compare

October 8, 2017 06:47

Ilya (iSazonov) reviewed

Oct 8, 2017

View reviewed changes

Comment thread

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

Ilya (iSazonov) Oct 8, 2017

Copy link

Copy Markdown

Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Please remove extra line.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Raghav S (iamraghavs) Oct 8, 2017

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Done as suggested

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

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

Ilya (iSazonov) Oct 8, 2017

Copy link

Copy Markdown

Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Please add braces here.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Raghav S (iamraghavs) Oct 8, 2017

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Added braces as suggested

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

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

Ilya (iSazonov) Oct 8, 2017

Copy link

Copy Markdown

Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Please use one line formatting as above.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Raghav S (iamraghavs) Oct 8, 2017

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Thanks for pointing that out. Done!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

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

Ilya (iSazonov) Oct 8, 2017

Copy link

Copy Markdown

Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Raghav S (iamraghavs) Oct 8, 2017

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

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

Ilya (iSazonov) Oct 8, 2017

Copy link

Copy Markdown

Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Raghav S (iamraghavs) Oct 8, 2017

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Raghav S (iamraghavs) Oct 8, 2017

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

removed the space

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Ilya (iSazonov) changed the title ~~Add NoNewLine switch for Out-String cmdlet (#3684)~~ Add NoNewLine switch for Out-String cmdlet

Oct 8, 2017

Raghav S (iamraghavs) force-pushed the fix-issue-3684 branch 2 times, most recently from d3a59c3 to f17122a Compare

October 8, 2017 14:59

Mark Kraus (markekraus) approved these changes

Oct 8, 2017

View reviewed changes

Mark Kraus (markekraus) left a comment

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

LGTM.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Mark Kraus (markekraus) commented Oct 8, 2017

Copy link

Copy Markdown

Contributor

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Ilya (iSazonov) reviewed

Oct 8, 2017

View reviewed changes

Comment thread

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

Ilya (iSazonov) Oct 8, 2017

Copy link

Copy Markdown

Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I'd prefer "NoNewlineFormatting".

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Raghav S (iamraghavs) Oct 9, 2017

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Nice. changing it now

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

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

Ilya (iSazonov) Oct 8, 2017

Copy link

Copy Markdown

Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Raghav S (iamraghavs) Oct 9, 2017

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Ilya (iSazonov) Oct 9, 2017

Copy link

Copy Markdown

Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Thanks! Yes, correct.

Closed.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

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

Ilya (iSazonov) Oct 8, 2017

Copy link

Copy Markdown

Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Raghav S (iamraghavs) Oct 9, 2017

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Ilya (iSazonov) added the Documentation Needed in this repo Documentation is needed in this repo label

Oct 8, 2017

Raghav S (iamraghavs) force-pushed the fix-issue-3684 branch from f17122a to cfd5ebd Compare

October 9, 2017 17:08

Dongbo Wang (daxian-dbw) requested changes

Oct 10, 2017

View reviewed changes

Comment thread

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

Dongbo Wang (daxian-dbw) Oct 10, 2017

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Raghav S (iamraghavs) Oct 10, 2017

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Sure. Done :)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Dongbo Wang (daxian-dbw) assigned Ilya (iSazonov)

Oct 10, 2017


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

eb5cb2a

Raghav S (iamraghavs) force-pushed the fix-issue-3684 branch from cfd5ebd to eb5cb2a Compare

October 10, 2017 06:34

Dongbo Wang (daxian-dbw) approved these changes

Oct 10, 2017

View reviewed changes

Dongbo Wang (daxian-dbw) left a comment •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Raghav S (iamraghavs) commented Oct 11, 2017

Copy link

Copy Markdown

Contributor Author

Dongbo Wang (@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 Michael Klement (@mklement0) Ilya (@iSazonov) Steve Lee (@SteveL-MSFT) . The comments were quite helpful!

mklement0 reacted with thumbs up emoji

All reactions

👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Ilya (iSazonov) commented Oct 11, 2017

Copy link

Copy Markdown

Collaborator

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

iamraghavs reacted with thumbs up emoji

All reactions

👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Ilya (iSazonov) merged commit 7dd36c9 into PowerShell:master

Oct 11, 2017

Ilya (iSazonov) commented Oct 11, 2017

Copy link

Copy Markdown

Collaborator

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

mklement0 reacted with thumbs up emoji

All reactions

👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Raghav S (iamraghavs) commented Oct 12, 2017

Copy link

Copy Markdown

Contributor Author

Yaay!

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Joey Aiello (joeyaiello) mentioned this pull request

Oct 15, 2018

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

Closed

Joey Aiello (joeyaiello) removed Documentation Needed in this repo Documentation is needed in this repo labels

Oct 15, 2018

Thatgfsj (Thatgfsj) pushed a commit to Thatgfsj-contribs/PowerShell that referenced this pull request

Aug 6, 2026


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

70c2d01

Reviewers

Ilya (iSazonov)

iSazonov left review comments

Dongbo Wang (daxian-dbw)

daxian-dbw approved these changes

Dan Travison (dantraMSFT)

Awaiting requested review from dantraMSFT

James Truher (JamesWTruher)

Awaiting requested review from JamesWTruher

Paul Higinbotham (PaulHigin)

Awaiting requested review from PaulHigin

+1 more reviewer

Mark Kraus (markekraus)

markekraus approved these changes

Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

Ilya (iSazonov)

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

6 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

Conversation

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

Uh oh!

Uh oh!

Microsoft Contribution License Agreements (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!

Mark Kraus (markekraus) left a comment

Choose a reason for hiding this comment

Uh oh!

Mark Kraus (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!

Dongbo Wang (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!

Raghav S (iamraghavs) commented Oct 11, 2017

Uh oh!

Ilya (iSazonov) commented Oct 11, 2017

Uh oh!

Ilya (iSazonov) commented Oct 11, 2017

Uh oh!

Raghav S (iamraghavs) commented Oct 12, 2017

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Raghav S (iamraghavs) commented Oct 8, 2017 •

edited by iSazonov

Loading

Uh oh!

There was an error while loading. Please reload this page.

Microsoft Contribution License Agreements (msftclas) commented Oct 8, 2017 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Dongbo Wang (daxian-dbw) left a comment •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.