-
Notifications
You must be signed in to change notification settings - Fork 8.1k
Get-Content -Raw should not neglects to read the last character #5076
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
Conversation
|
@mklement0 Could you please review? |
| string contentString = content.ToString(); | ||
| blocks.Add( | ||
| !readBackward && contentString.EndsWith(actualDelimiter, StringComparison.Ordinal) | ||
| !readBackward && contentString.EndsWith(actualDelimiter, StringComparison.Ordinal) && !_isRawStream |
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.
This looks good;
but original issue says that "Note that Windows Powershell is not affected".
Any ideas why this does not repro in Windows PS?
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.
This is because the code is introduced by core ps code change.
| } | ||
| } | ||
|
|
||
| Describe "Get-Content -Raw test" -Tags "CI" { |
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.
This test code is a good candidate to be rewritten using -testcases construct, as per testing guidelines.
|
@anmenaga your comments are resolved. thanks |
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 move to It block:
It "Reads - <testname> in full" -TestCases @(
@{character = "a`nb`n"; testname = "LF-terminated files"; filename = "lf.txt"}
....
) {
....
}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 your comment. But I would rather not change it as it is just format preference and the submitted format is following the testing guidelines example.
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.
It is new code not formatting. We should place it in a right place. The right place is either It block (sample) or BeforeAll block. We use the set only one time so It block is best place. This does not contradict the testing guidelines.
|
@chunqingchen Thanks for the fix! |
resolve #4980
once the -raw parameter is set, we should force the contentString not to substring under any circumstance
Before the fix:
get-content -raw will ignore \n
After the fix:
get-content -raw will get all the contents include \n