Skip to content
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.

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

Fixed #58 - Newlines in commands render incorrectly #60

Merged
merged 5 commits into from
Mar 12, 2020

Conversation

tig
Copy link
Collaborator

@tig tig commented Mar 11, 2020

Per #58

// Replace any newlines with encoded newline (`n)
// Note we can't use Environment.Newline because we don't know that the
// Command honors that.
strings[i] = strings[i].Replace("\n", "`n");
Copy link
Member

Choose a reason for hiding this comment

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

So thinking about this some more, it may be possible that the string has a carriage return and a linefeed. So in addition to what you're doing here, we should also handle that so:

Suggested change
strings[i] = strings[i].Replace("\n", "`n");
strings[i] = strings[i].Replace("\r\n", "`r`n");
strings[i] = strings[i].Replace("\n", "`n");

This makes sure there are no lingering \r.

Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be just as good to do a replace for \r and another for \n and thus catch all of them by default?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah that's true. I guess that would catch the off chance that there's a \r by itself.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I concur. I've changed it to:

                // Replace any newlines with encoded newline/linefeed (`n or `r)
                // Note we can't use Environment.Newline because we don't know that the
                // Command honors that.
                strings[i] = strings[i].Replace("\r", "`r");
                strings[i] = strings[i].Replace("\n", "`n");

I don't do many PRs, so I'm not sure what I'm supposed to do to push this through other than pushing it to my master branch... I think it's automatic?

Choose a reason for hiding this comment

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

You should create new branch for every new PR.

tig and others added 2 commits March 11, 2020 16:30
Copy link
Member

@TylerLeonhardt TylerLeonhardt left a comment

Choose a reason for hiding this comment

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

LGTM awesome work!

@TylerLeonhardt
Copy link
Member

I'll give @SteveL-MSFT a chance to review since I won't be snapping a new release til probably Friday/Monday.

Copy link
Member

@TylerLeonhardt TylerLeonhardt left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -88,6 +88,35 @@ Get-Process | Out-ConsoleGridView
> NOTE: If you change the code and rebuild the project, you'll need to launch a
> _new_ PowerShell process since the dll is already loaded and can't be unloaded.

### Debugging in Visual Studio Code
Copy link
Member

Choose a reason for hiding this comment

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

Thank you so much for this!!

@corbob
Copy link
Contributor

corbob commented Mar 12, 2020

I don't do many PRs, so I'm not sure what I'm supposed to do to push this through other than pushing it to my master branch... I think it's automatic?

@tig by pushing to the branch for the PR it will automatically update the PR. A general recommendation is to create a branch for the PR and push to that branch. This allows you to keep master up to date with the upstream repository without causing issues with your PR (it also appears to be fairly common for the first few PRs to do them from master because it's not known how else to do it).

@tig
Copy link
Collaborator Author

tig commented Mar 12, 2020

I don't do many PRs, so I'm not sure what I'm supposed to do to push this through other than pushing it to my master branch... I think it's automatic?

@tig by pushing to the branch for the PR it will automatically update the PR. A general recommendation is to create a branch for the PR and push to that branch. This allows you to keep master up to date with the upstream repository without causing issues with your PR (it also appears to be fairly common for the first few PRs to do them from master because it's not known how else to do it).

Yep. I now get it. Appreciate the help.

Copy link
Member

@SteveL-MSFT SteveL-MSFT left a comment

Choose a reason for hiding this comment

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

LGTM, great addition. Thanks for your contribution!

@TylerLeonhardt TylerLeonhardt merged commit ebd74fd into PowerShell:master Mar 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants