Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Fix 'Get-Content -Delimiter' to not include the delimiter in the returned lines#3808

Merged
daxian-dbw merged 2 commits into
PowerShell:masterPowerShell/PowerShell:masterfrom
mklement0:get-content-delimiter-fixmklement0/PowerShell:get-content-delimiter-fixCopy head branch name to clipboard
Sep 1, 2017
Merged

Fix 'Get-Content -Delimiter' to not include the delimiter in the returned lines#3808
daxian-dbw merged 2 commits into
PowerShell:masterPowerShell/PowerShell:masterfrom
mklement0:get-content-delimiter-fixmklement0/PowerShell:get-content-delimiter-fixCopy head branch name to clipboard

Conversation

@mklement0

@mklement0 mklement0 commented May 18, 2017 •
edited by daxian-dbw
Loading

Copy link
Copy Markdown
Contributor

Fix #3706

@msftclas

msftclas commented May 18, 2017

Copy link
Copy Markdown

@mklement0,
Thanks for having already signed the Contribution License Agreement. Your agreement was validated by Microsoft. We will now review your pull request.
Thanks,
Microsoft Pull Request Bot

iSazonov
iSazonov reviewed May 18, 2017
View reviewed changes

@iSazonov iSazonov left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@mklement0 It seems you don't fix "tail".

/cc @jeffbi Could you please review?

@iSazonov iSazonov requested a review from jeffbi May 18, 2017 11:03
@mklement0

mklement0 commented May 18, 2017

Copy link
Copy Markdown
Contributor Author

@iSazonov Thanks for catching the -tail issue - oddly, the tests pass on macOS (which is where I ran them before submitting the PR). I'll look into it.

@iSazonov

iSazonov commented May 18, 2017

Copy link
Copy Markdown
Collaborator

I believe the problem is in platform difference of Newline.

@mirichmo mirichmo self-assigned this May 24, 2017
adityapatwardhan
adityapatwardhan requested changes May 24, 2017
View reviewed changes
Comment thread test/powershell/Modules/Microsoft.PowerShell.Management/Get-Content.Tests.ps1 Outdated

@adityapatwardhan adityapatwardhan May 24, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can this be converted to use the -TestCase parameter for It? That would make the test much more readable.

@mklement0 mklement0 May 24, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I didn't write these tests (and the only modification I made was to resolve aliases to their full cmdlet names), but I can look into it.

@SteveL-MSFT SteveL-MSFT Jun 1, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Rather than change it here, I suggest we open a new issue to refactor these test cases to use -TestCase

@mirichmo mirichmo Jun 6, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I filed Issue #3945 to cover this comment.

Comment thread test/powershell/Modules/Microsoft.PowerShell.Management/Get-Content.Tests.ps1 Outdated

@adityapatwardhan adityapatwardhan May 24, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Any reason to skip on Linux or OSX?

@mklement0 mklement0 May 24, 2017 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Again, I can't speak to these tests, but perhaps BugId(BugDatabase.WindowsOutOfBandReleases, 906022) holds the key?

Separately, if you have any pointers as to why the PR doesn't work on Windows (have yet to dig into it), that would be helpful.

@iSazonov thinks that it may be the difference in newline sequences, though that's not immediately obvious to me.

@SteveL-MSFT SteveL-MSFT Jun 1, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

unfortunately, I don't know if that bug database even exists anymore. that comment is basically worthless now.

@mirichmo mirichmo mentioned this pull request Jun 6, 2017
Closed
@mirichmo

mirichmo commented Jun 6, 2017

Copy link
Copy Markdown
Member

@adityapatwardhan I don't see any more outstanding comments. Do you have any additional concerns?

@mirichmo

mirichmo commented Jun 6, 2017

Copy link
Copy Markdown
Member

@mklement0 Please address the failing test:

[-] should Get-Content with a variety of -Tail and -ReadCount values 68ms
Expected: {2}
But was: {3}
at line: 104 in C:\projects\powershell\test\powershell\Modules\Microsoft.PowerShell.Management\Get-Content.Tests.ps1
104: $result.Length | Should Be 2

@mklement0

mklement0 commented Jun 7, 2017

Copy link
Copy Markdown
Contributor Author

@mirichmo: I'll take a look soon. In the meantime, if you have a high-level pointer as to how the platform-specific newline sequence might come into play here (it works in the Unix world, but not on Windows), please let me know.

@iSazonov

iSazonov commented Aug 24, 2017 •
edited
Loading

Copy link
Copy Markdown
Collaborator

@mklement0 Test failed for -Tail parameter. If we use the parameter we call ReadDelimited() twice - (1) to seek start position - backward read, (2) read forward from the start position and parse the file. In the case we can not simply remove the delimiter character from the stream because we use it in SeekItemsBackward() to correct start position. It seems there is still problem places - leave the research to you 😄 Please continue the PR.

Also I see the code is not optimal and we should refactor this in another PR.

@mklement0

mklement0 commented Aug 26, 2017

Copy link
Copy Markdown
Contributor Author

Thanks for the tips, @iSazonov.
I hope to get to this soon (I know, deja vu).

@mklement0 mklement0 force-pushed the get-content-delimiter-fix branch from 65c0a5f to 906a276 Compare August 30, 2017 19:40
@mklement0

mklement0 commented Aug 30, 2017

Copy link
Copy Markdown
Contributor Author

Note that the only changes to Get-Content.Tests.ps1 since the original commit were whitespace normalization (conversion to all-spaces, though I now realize that there are some inconsistencies).

Thanks to @iSazonov for pointing me to what I hope is the right fix, which turned out to be simple in the end.

iSazonov
iSazonov reviewed Aug 31, 2017
View reviewed changes
Comment thread src/System.Management.Automation/namespaces/FileSystemContentStream.cs Outdated

@iSazonov iSazonov Aug 31, 2017

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Here we evaluate content.ToString() twice - maybe do this one before block.Add().

@iSazonov

iSazonov commented Aug 31, 2017

Copy link
Copy Markdown
Collaborator

@mklement0 Could you please split functional and formatting changes to two commits? This will help us avoid error skipping.
Use soft reset, then git add -p.

@mklement0 mklement0 force-pushed the get-content-delimiter-fix branch from 906a276 to c50f3f8 Compare August 31, 2017 18:16
@mklement0

mklement0 commented Aug 31, 2017

Copy link
Copy Markdown
Contributor Author

@iSazonov: I've made the changes and split them into 2 commits (functional vs. formatting) that replaced the earlier ones.

I think the Travis CI failure is a transient one, unrelated to my changes (it got stuck while setting up the environment).

Is there a way to re-initiate the test?

@SteveL-MSFT

SteveL-MSFT commented Aug 31, 2017

Copy link
Copy Markdown
Member

@mklement0 I restarted that job for you

@daxian-dbw daxian-dbw self-assigned this Sep 1, 2017
@iSazonov

iSazonov commented Sep 1, 2017 •
edited
Loading

Copy link
Copy Markdown
Collaborator

@mklement0 Tip: You can "rename" last commit to restart CI jobs.

@mklement0

mklement0 commented Sep 1, 2017

Copy link
Copy Markdown
Contributor Author

Thanks, @iSazonov, but how do you rename a commit? Amend the commit message and push again?

@daxian-dbw

daxian-dbw commented Sep 1, 2017

Copy link
Copy Markdown
Member

Thanks all! Since we got 2 approvals and @iSazonov's comments are also addressed, I will rebase and merge this PR soon (in about one hour). Please let me know if there are any concerns.

@daxian-dbw daxian-dbw added the Breaking-Change breaking change that may affect users label Sep 1, 2017
@daxian-dbw daxian-dbw changed the title Fix for Get-Content -Delimiter including the delimiter in the array … Fix 'Get-Content -Delimiter' to not include the delimiter in the returned lines Sep 1, 2017
@daxian-dbw daxian-dbw merged commit 803395b into PowerShell:master Sep 1, 2017
@mklement0 mklement0 deleted the get-content-delimiter-fix branch September 1, 2017 20:30
@iSazonov

iSazonov commented Sep 2, 2017

Copy link
Copy Markdown
Collaborator

@mklement0 Yes, you can amend or rebase+reword.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@iSazonov iSazonov iSazonov left review comments

@SteveL-MSFT SteveL-MSFT SteveL-MSFT approved these changes

@adityapatwardhan adityapatwardhan adityapatwardhan approved these changes

@jeffbi jeffbi Awaiting requested review from jeffbi

@anmenaga anmenaga Awaiting requested review from anmenaga

@BrucePay BrucePay Awaiting requested review from BrucePay

+1 more reviewer

@mirichmo mirichmo mirichmo left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@daxian-dbw daxian-dbw

Labels

Breaking-Change breaking change that may affect users

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@mklement0 @msftclas @iSazonov @mirichmo @SteveL-MSFT @daxian-dbw @adityapatwardhan
Morty Proxy This is a proxified and sanitized view of the page, visit original site.