-
Notifications
You must be signed in to change notification settings - Fork 347
Implement DataFrame.LoadCsvFromString #2988
Conversation
/// <param name="encoding">The character encoding. Defaults to UTF8 if not specified</param> | ||
/// <returns><see cref="DataFrame"/></returns> | ||
public static DataFrame LoadCsv(Stream csvStream, | ||
private static DataFrame ReadCsvLinesIntoDataFrame(IEnumerable<string> lines, |
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 method is just a refactor of the logic in LoadCsv
with 1 simple change: The first parameter here is an IEnumerable<string>
.
|
||
using (var streamReader = new StreamReader(csvStream, encoding ?? Encoding.UTF8, detectEncodingFromByteOrderMarks: true, DefaultStreamReaderBufferSize, leaveOpen: true)) | ||
{ | ||
CsvLineEnumerator linesEnumerator = new CsvLineEnumerator(streamReader); |
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.
Just a couple structs to wrap a StreamReader
within an IEnumerable<string>
.
long numberOfRowsToRead = -1, int guessRows = 10, bool addIndexColumn = false, | ||
Encoding encoding = null) | ||
{ | ||
string[] lines = csvString.Split(new[] { Environment.NewLine }, StringSplitOptions.None); |
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.
Are we just completely giving up on quoting? Or is this just a "first effort" to get the API in?
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 just a "first effort". Or more accurately the easiest way to get this API in with just some minor refactoring. I'll work on a proper csv parser shortly and re-visit this implementation at that point.
|
||
public void Dispose() | ||
{ | ||
throw new NotImplementedException(); |
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.
Why throw on Dispose?
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.
I put that in so we catch it if someone calls this method. We're not dealing with any unmanaged resources here, so no one should be calling Dispose
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.
LGTM
Fixes #2984