www.digitalmars.com         C & C++   DMDScript  

digitalmars.D - Reminder: Two days left to review Jesse Phillips' CSV Parser

reply dsimcha <dsimcha yahoo.com> writes:
As a reminder, the review of Jesse Phillips' CSV parser ends at the end 
of Friday and will be followed by one week of voting.  Please speak up 
now about any remaining issues.
Nov 09 2011
next sibling parent reply dsimcha <dsimcha yahoo.com> writes:
On 11/9/2011 8:02 PM, dsimcha wrote:

 As a reminder, the review of Jesse Phillips' CSV parser ends at the end
 of Friday and will be followed by one week of voting. Please speak up
 now about any remaining issues.


BTW, here's my second round of review now that some changes have been made:

Design-level stuff:

How do I use csvReader to process a CSV file as it's being read in (i.e. 
I don't want to ever have the whole file in memory)?  I guess I could do 
something like:

import std.algorithm, std.stdio, std.csv, std.conv;

void main() {
     auto file = File("foo.csv");
     auto chars = joiner(file.byLine(), '\n');

     auto reader = csvReader(chars);
}

If this would work, though, then it should be documented.  I'd also like 
to see csvReader tested with lowest-common-denominator input ranges 
instead of just strings, which are bidirectional ranges and can be 
treated as random access if they only have ASCII characters.  Also, IMHO 
we really need a ByChar range in std.stdio instead of the joiner kludge 
I used, though I'm not sure about the details with regard to how Unicode 
support should work.

Still no support for writing CSVs?  I understand writing is complicated 
from an API design perspective.  Maybe it would be easiest to just 
provide an enquote() function that adds quotes to a field and escapes 
any that are already inside the field and let the user handle the higher 
level stuff, since this is pretty simple.

Minor documentation nitpicks:

CSVException:  IncompletCellException -> IncompleteCellException (typo)

The first csvReader example:  You declare the variable count but don't 
use it.

Records, Record:  front(), empty(), popFront() should not mention 
std.range.InputRange.  These interfaces are virtual function-based 
interfaces in case someone needs virtual function-based access to a 
range.  They are not used in most cases when ranges are used because 
templates are usually better.
Nov 09 2011
parent Jesse Phillips <jessekphillips+d gmail.com> writes:
On Wed, 09 Nov 2011 20:46:51 -0500, dsimcha wrote:


 On 11/9/2011 8:02 PM, dsimcha wrote:

 As a reminder, the review of Jesse Phillips' CSV parser ends at the end
 of Friday and will be followed by one week of voting. Please speak up
 now about any remaining issues.


 
 BTW, here's my second round of review now that some changes have been
 made:
 
 Design-level stuff:
 
 How do I use csvReader to process a CSV file as it's being read in (i.e.
 I don't want to ever have the whole file in memory)?  I guess I could do
 something like:
 
 import std.algorithm, std.stdio, std.csv, std.conv;
 
 void main() {
      auto file = File("foo.csv");
      auto chars = joiner(file.byLine(), '\n');
 
      auto reader = csvReader(chars);
 }
 
 If this would work, though, then it should be documented.


I do not see why the CSV documentation should be the place to document 
how to read a file in parts. As we don't have byChar and this is non-
trivial workaround I could see wanted to have it somewhere and CSV 
parsing might be a place to apply it.

What really don't like about it is that you don't get to see the input. 
It would be like std.algorithm showing how sorting worked by showing how 
to read in an array from a file and sorting it.

This is something people need to know how to do so we should have a good 
tutorial on it, but I don't think this is the place to document it (stdio 
or std.file is more appropriate). If you strongly disagree I will find 
some place to throw in another example.


 I'd also like
 to see csvReader tested with lowest-common-denominator input ranges
 instead of just strings, which are bidirectional ranges and can be
 treated as random access if they only have ASCII characters.


Yeah, through in a unittest wrapping a wstring with only input range 
interface.


 Also, IMHO
 we really need a ByChar range in std.stdio instead of the joiner kludge
 I used, though I'm not sure about the details with regard to how Unicode
 support should work.


I thought there was an undocumented such function, but maybe not.
 

 Still no support for writing CSVs?  I understand writing is complicated
 from an API design perspective.  Maybe it would be easiest to just
 provide an enquote() function that adds quotes to a field and escapes
 any that are already inside the field and let the user handle the higher
 level stuff, since this is pretty simple.


I'm going to make something, try a couple ideas out. Thanks for the 
naming suggestion. I ended up deciding not to do anything on it as even 
if I did get it done, it wouldn't have appropriate review time. So I'll 
let the voting be the deciding factor on whether the library is delayed 
because it doesn't have csvFormatter.

After more thought I think an enquote() type function must exist even 
with a csvFormatter, but even laying out documentation on that isn't 
trivial.


 Minor documentation nitpicks:
 
 CSVException:  IncompletCellException -> IncompleteCellException (typo)
 
 The first csvReader example:  You declare the variable count but don't
 use it.
 
 Records, Record:  front(), empty(), popFront() should not mention
 std.range.InputRange.  These interfaces are virtual function-based
 interfaces in case someone needs virtual function-based access to a
 range.  They are not used in most cases when ranges are used because
 templates are usually better.


Yes, my bad. I keep forgetting we have such interfaces there. I've 
instead pointed it to isInputRange which is what I meant to do. But then 
I still call it an interface... which it is, but I don't know what we 
want to call them. Concepts? :D I want this link in there so if you have 
better suggestions...

Thank you.
Nov 09 2011
prev sibling next sibling parent reply Jonas Drewsen <jdrewsen nospam.com> writes:
On 10/11/11 02.02, dsimcha wrote:

 As a reminder, the review of Jesse Phillips' CSV parser ends at the end
 of Friday and will be followed by one week of voting. Please speak up
 now about any remaining issues.


I've not have a look at the code itself yet. But a couple of comments 
about the API:

auto csvReader(Contents = string, Range, Heading, Separator = 
char)(Range input, Heading heading, Separator delimiter = ',', Separator 
quote = '"');

1, I was under the impression that declaring the return value as auto 
for std library is not good because it does not document what the return 
value actually is. Or maybe it is good enough that it is mentioned in 
the comments under "Returns: ..."?

2, The requirement for passing "cast(string[]) null" as the heading 
parameter to signal that heading is there but should be ignored seems a 
bit awkward.
Maybe an overload for csvReader where you remove the constraints: 
isForwardRange!Heading && isSomeString!(ElementType!Heading) and set 
is(Heading:void*) instead. And in there just call the normal csvReader 
with cast(string[])null.

Btw. if any of this has already been brought up then sorry about that.

/Jonas
Nov 10 2011
next sibling parent reply "Jonathan M Davis" <jmdavisProg gmx.com> writes:
On Thursday, November 10, 2011 06:04 Jonas Drewsen wrote:

 On 10/11/11 02.02, dsimcha wrote:

 As a reminder, the review of Jesse Phillips' CSV parser ends at the end
 of Friday and will be followed by one week of voting. Please speak up
 now about any remaining issues.


 
 I've not have a look at the code itself yet. But a couple of comments
 about the API:
 
 auto csvReader(Contents = string, Range, Heading, Separator =
 char)(Range input, Heading heading, Separator delimiter = ',', Separator
 quote = '"');
 
 1, I was under the impression that declaring the return value as auto
 for std library is not good because it does not document what the return
 value actually is. Or maybe it is good enough that it is mentioned in
 the comments under "Returns: ..."?


Then go look at std.algorithm. It returns auto all over the place. That's 
usually what you do when returning a new range type. The documentation then 
says that it's returning a range. And that's essentially what the docs do in 
this case. It would be horrible to have to give the return type for many 
functions that return ranges. They're frequently hideous templates that would 
just scare people (which is how std.algorithm's documentation used to look). 
All you need to know is that it's a range, not what the exact type is.

- Jonathan M Davis
Nov 10 2011
parent Jonas Drewsen <jdrewsen nospam.com> writes:
Den 10-11-2011 19:05, Jonathan M Davis skrev:

 On Thursday, November 10, 2011 06:04 Jonas Drewsen wrote:

 On 10/11/11 02.02, dsimcha wrote:

 As a reminder, the review of Jesse Phillips' CSV parser ends at the end
 of Friday and will be followed by one week of voting. Please speak up
 now about any remaining issues.


 I've not have a look at the code itself yet. But a couple of comments
 about the API:

 auto csvReader(Contents = string, Range, Heading, Separator =
 char)(Range input, Heading heading, Separator delimiter = ',', Separator
 quote = '"');

 1, I was under the impression that declaring the return value as auto
 for std library is not good because it does not document what the return
 value actually is. Or maybe it is good enough that it is mentioned in
 the comments under "Returns: ..."?


 Then go look at std.algorithm. It returns auto all over the place. That's
 usually what you do when returning a new range type. The documentation then
 says that it's returning a range. And that's essentially what the docs do in
 this case. It would be horrible to have to give the return type for many
 functions that return ranges. They're frequently hideous templates that would
 just scare people (which is how std.algorithm's documentation used to look).
 All you need to know is that it's a range, not what the exact type is.

 - Jonathan M Davis


Ok - better go change curl range functions back to auto then then :)
Nov 10 2011
prev sibling parent reply Jesse Phillips <jessekphillips+d gmail.com> writes:
On Thu, 10 Nov 2011 15:04:47 +0100, Jonas Drewsen wrote:


 On 10/11/11 02.02, dsimcha wrote:

 As a reminder, the review of Jesse Phillips' CSV parser ends at the end
 of Friday and will be followed by one week of voting. Please speak up
 now about any remaining issues.


 
 I've not have a look at the code itself yet. But a couple of comments
 about the API:
 
 auto csvReader(Contents = string, Range, Heading, Separator =
 char)(Range input, Heading heading, Separator delimiter = ',', Separator
 quote = '"');
 
 1, I was under the impression that declaring the return value as auto
 for std library is not good because it does not document what the return
 value actually is. Or maybe it is good enough that it is mentioned in
 the comments under "Returns: ..."?


Quite simply, I don't know what it returns. The discussion was on 
examples using auto, which I also do for basically the same reason. 
Everyone was in agreement that complicated template returns deserve the 
use of auto, but I didn't get an answer why _complicated_ types get auto 
but simple ones can't use it. Luckily mine is complicated.
 

 2, The requirement for passing "cast(string[]) null" as the heading
 parameter to signal that heading is there but should be ignored seems a
 bit awkward.
 Maybe an overload for csvReader where you remove the constraints:
 isForwardRange!Heading && isSomeString!(ElementType!Heading) and set
 is(Heading:void*) instead. And in there just call the normal csvReader
 with cast(string[])null.


You should be able to pass null, but there is a bug in dmd. You might 
have a good workaround for that though.

Well, it seems I just have to assume that void* means they are passing 
null. Thank you.
 

 Btw. if any of this has already been brought up then sorry about that.
 
 /Jonas
Nov 10 2011
parent Timon Gehr <timon.gehr gmx.ch> writes:
On 11/11/2011 06:49 AM, Jesse Phillips wrote:

 On Thu, 10 Nov 2011 15:04:47 +0100, Jonas Drewsen wrote:


 On 10/11/11 02.02, dsimcha wrote:

 As a reminder, the review of Jesse Phillips' CSV parser ends at the end
 of Friday and will be followed by one week of voting. Please speak up
 now about any remaining issues.


 I've not have a look at the code itself yet. But a couple of comments
 about the API:

 auto csvReader(Contents = string, Range, Heading, Separator =
 char)(Range input, Heading heading, Separator delimiter = ',', Separator
 quote = '"');

 1, I was under the impression that declaring the return value as auto
 for std library is not good because it does not document what the return
 value actually is. Or maybe it is good enough that it is mentioned in
 the comments under "Returns: ..."?


 Quite simply, I don't know what it returns. The discussion was on
 examples using auto, which I also do for basically the same reason.
 Everyone was in agreement that complicated template returns deserve the
 use of auto, but I didn't get an answer why _complicated_ types get auto
 but simple ones can't use it. Luckily mine is complicated.


 2, The requirement for passing "cast(string[]) null" as the heading
 parameter to signal that heading is there but should be ignored seems a
 bit awkward.
 Maybe an overload for csvReader where you remove the constraints:
 isForwardRange!Heading&&  isSomeString!(ElementType!Heading) and set
 is(Heading:void*) instead. And in there just call the normal csvReader
 with cast(string[])null.


 You should be able to pass null, but there is a bug in dmd. You might
 have a good workaround for that though.

 Well, it seems I just have to assume that void* means they are passing
 null. Thank you.


Kenji has already written a pull request to give null it's own type. 
Therefore, make sure not to hardcode void*, use typeof(null).
Nov 11 2011
prev sibling parent Dejan Lekic <dejan.lekic gmail.com> writes:
dsimcha wrote:


 As a reminder, the review of Jesse Phillips' CSV parser ends at the end
 of Friday and will be followed by one week of voting.  Please speak up
 now about any remaining issues.


I humbly believe the package for CSV parser is wrong. It should either be 
std.file.format.csv, std.parser.csv or (my personal preference) 
std.data.storage.csv . Why I prefer the latest? - CSV is ultimately data 
storage format. OK, it is often used for *data* exchange, but even then it 
is a *data-storage*. The fact that MariaDB has it as a storage (CSV storage 
engine) does not add weight to my argument, but it supports what I am trying 
to say. :) std.csv is just wrong. I know, it is nice and short, less typing 
etc, but still wrong.

Sure this really is irrelevant to this discussion as csv module can be moved 
around any time. :)

Kind regards
Nov 10 2011