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

mklement0 · 2017-05-18T04:55:21Z

msftclas · 2017-05-18T04:55:25Z

@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

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

/cc @jeffbi Could you please review?

mklement0 · 2017-05-18T16:56:56Z

@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 · 2017-05-18T18:31:21Z

I believe the problem is in platform difference of Newline.

adityapatwardhan · 2017-05-24T16:50:43Z

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

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

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.

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

I filed Issue #3945 to cover this comment.

adityapatwardhan · 2017-05-24T17:00:33Z

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

Any reason to skip on Linux or OSX?

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.

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

mirichmo · 2017-06-06T15:30:04Z

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

mirichmo · 2017-06-06T18:35:22Z

@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 · 2017-06-07T01:58:20Z

@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 · 2017-08-24T15:24:25Z

@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 · 2017-08-26T03:07:05Z

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

mklement0 · 2017-08-30T20:16:36Z

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 · 2017-08-31T05:18:59Z

src/System.Management.Automation/namespaces/FileSystemContentStream.cs

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

iSazonov · 2017-08-31T05:30:39Z

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

…lements returned (PowerShell#3706) - functional changes

…lements returned (PowerShell#3706) - formatting changes

mklement0 · 2017-08-31T20:58:48Z

@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 · 2017-08-31T21:38:43Z

@mklement0 I restarted that job for you

iSazonov · 2017-09-01T17:59:11Z

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

mklement0 · 2017-09-01T18:01:29Z

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

daxian-dbw · 2017-09-01T18:49:55Z

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.

iSazonov · 2017-09-02T16:45:55Z

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

msftclas added the cla-already-signed label May 18, 2017

iSazonov reviewed May 18, 2017

View reviewed changes

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

mirichmo self-assigned this May 24, 2017

mirichmo requested review from SteveL-MSFT and adityapatwardhan May 24, 2017 00:37

adityapatwardhan requested changes May 24, 2017

View reviewed changes

mirichmo mentioned this pull request Jun 6, 2017

Refactor the Get-Content tests to use -TestCases #3945

Closed

adityapatwardhan approved these changes Jun 6, 2017

View reviewed changes

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

mklement0 requested review from BrucePay and anmenaga as code owners August 30, 2017 19:40

SteveL-MSFT approved these changes Aug 30, 2017

View reviewed changes

iSazonov reviewed Aug 31, 2017

View reviewed changes

mklement0 added 2 commits August 31, 2017 13:56

Fix for Get-Content -Delimiter including the delimiter in the array e…

7fb865f

…lements returned (PowerShell#3706) - functional changes

Fix for Get-Content -Delimiter including the delimiter in the array e…

c50f3f8

…lements returned (PowerShell#3706) - formatting changes

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

daxian-dbw unassigned mirichmo Sep 1, 2017

daxian-dbw self-assigned this Sep 1, 2017

daxian-dbw added the Breaking-Change breaking change that may affect users label Sep 1, 2017

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 merged commit 803395b into PowerShell:master Sep 1, 2017

mklement0 deleted the get-content-delimiter-fix branch September 1, 2017 20:30

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

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

Uh oh!

Conversation

mklement0 commented May 18, 2017 • edited by daxian-dbw Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

msftclas commented May 18, 2017

Uh oh!

iSazonov left a comment

Choose a reason for hiding this comment

Uh oh!

mklement0 commented May 18, 2017

Uh oh!

iSazonov commented May 18, 2017

Uh oh!

adityapatwardhan May 24, 2017

Choose a reason for hiding this comment

Uh oh!

mklement0 May 24, 2017

Choose a reason for hiding this comment

Uh oh!

SteveL-MSFT Jun 1, 2017

Choose a reason for hiding this comment

Uh oh!

mirichmo Jun 6, 2017

Choose a reason for hiding this comment

Uh oh!

adityapatwardhan May 24, 2017

Choose a reason for hiding this comment

Uh oh!

mklement0 May 24, 2017 • 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!

SteveL-MSFT Jun 1, 2017

Choose a reason for hiding this comment

Uh oh!

mirichmo commented Jun 6, 2017

Uh oh!

mirichmo commented Jun 6, 2017

Uh oh!

mklement0 commented Jun 7, 2017

Uh oh!

iSazonov commented Aug 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mklement0 commented Aug 26, 2017

Uh oh!

mklement0 commented Aug 30, 2017

Uh oh!

iSazonov Aug 31, 2017

Choose a reason for hiding this comment

Uh oh!

iSazonov commented Aug 31, 2017

Uh oh!

mklement0 commented Aug 31, 2017

Uh oh!

SteveL-MSFT commented Aug 31, 2017

Uh oh!

iSazonov commented Sep 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mklement0 commented Sep 1, 2017

Uh oh!

daxian-dbw commented Sep 1, 2017

Uh oh!

iSazonov commented Sep 2, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

mklement0 commented May 18, 2017 •

edited by daxian-dbw

Loading

mklement0 May 24, 2017 •

edited

Loading

iSazonov commented Aug 24, 2017 •

edited

Loading

iSazonov commented Sep 1, 2017 •

edited

Loading