Add support LF newlines in Import-Csv#5363
Uh oh!
There was an error while loading. Please reload this page.
Conversation
iSazonov requested review from JamesWTruher, PaulHigin and dantraMSFT as code owners
markekraus commented Nov 7, 2017
@iSazonov are there existing test cases for all three line ending scenarios? |
Yes, I added new test. Oops... lost commit. |
| returnch=='\r'||ch=='\n'; | ||
| } | ||
| private |
There was a problem hiding this comment.
We
should
fix
this.
But thanks for being consistent. :)
There was a problem hiding this comment.
Can we get a good project wide reformatting done right after release? So many times I'm just following the ugly formatting around the code and then have to point that out in reviews as to why my code looks weird or doesn't follow the conventions.
There was a problem hiding this comment.
I ran the Roslyn code format tool on the code base before we went public - it didn't fix stuff like this, but we should run it again.
There was a problem hiding this comment.
Maybe just after RTM take two formatting weeks to push as many formatting PRs as we can starting with Roslyn code format tool and continue with manual formatting.
| { | ||
| ReadChar(); | ||
| } | ||
| SkipNewLine(); |
There was a problem hiding this comment.
If there are two newlines, won't this skip the second?
A better pattern might be to change IsNewLine to return a string ("\n", "\r", or "\r\n") or null, and if the newline must be appended, append the result. There should be no need for SkipNewLine.
There was a problem hiding this comment.
I don't know about skipping empty lines. Formally RFC4180 don't allow empty lines. Although practically it may make sense.
I thought about the pattern ( IsNewLine to return a string ) and decided to begin with SkipNewLine() as seems more readable. Now I'm going to implement IsNewLine(out newLine).
There was a problem hiding this comment.
I test with Windows PowerShell - we keep its behaviour and ignore empty lines.
Microsoft Excel imports empty lines as empty rows. We should discuss this in new Issue if we want because it is a breaking change.
4 participants
Fix#3692.
Fix description
After the fix Import-Csv support CR (\r), LF (\n), CRLF (\r\n) as line delimiters.