Review of Jesse Phillips's CSV Parser

[Jesse Phillips]

Thank you all.

I've made my first round of updates to the documentation, will be taking 
another pass and looking at adding to functionality. I'd like some help 
with Walters comments as described below.

On Fri, 28 Oct 2011 12:18:47 -0700, Walter Bright wrote:

> On 10/28/2011 6:18 AM, dsimcha wrote:
>> Formal review of Jesse Phillips's CSV parser module begins today and
>> runs through November . Following that, a vote will take place for one
>> week. Please post any comments about this library in this thread.
>>
>> Docs:
>> http://nascent.freeshell.org/programming/D/doc/phobos/std_csv.html
>>
>> Code:
>> https://github.com/he-the-great/phobos/tree/csv
> 
> First off, mucho thanks to Jesse for writing this. It's an important
> module for Phobos.

Thank you. Fun fact, many popular languages don't include a CSV parser, 
C# and Java being notable ones (or at least I haven't found it).

> This is a review of the documentation:
> 
> Include this link: http://en.wikipedia.org/wiki/Comma-separated_values
> so the programmer can learn more about CSV.

Done, added See Also section.
 
> "This parser will loosely follow the RFC-4180"
> 
> How exactly will it differ from RFC-4180?

I do not know how to address this. After making this statement I list the 
criteria. I don't see the need to list where it differs:

* Records terminate with LF, CR and not just CRLF.
* Using Double Quotes or Comma can be customized.

I feel that I summarized well enough that listing yet another would be 
redundant.
 
> "No exceptions are thrown due to incorrect CSV."
> 
> What happens, then, when the CSV is incorrect?

Added list.

> "csvText"
> 
> Be more specific about what csvText() returns.

You are right. csvText() return a RecordList, but I explain it as though 
it were the Range. I'll think about what detail here, any suggestions are 
welcome.
 
> I'm confused about the heading parameter to csvText().

Well I hope some reorganizing and an added example will help.
 
> "the Content of the input"
>
> What is that? There's no parameter named "Content".

I've replace all instances of Content with Contents. It is a template 
parameter so hopefully this resolves the confusion.
 
> "RecordList"
> 
> "Range which provides access to CSV Records and Fields."
> 
> What is the Range? Do you mean RecordList is a Range? And what does
> "provides access" mean? If RecordList is a Range, why is it labeled a
> "List"?

Considering it is the first documentation line of RecordList, I don't see 
where the confusion on "what the range is."

I've re-written the line, it's shorter.

Ok, be prepared for confusion.

RecordList is a range of records, it is not named RecordRange because 
Record is also a range, a range of fields. If you have any suggestions 
for a better name, such as

Document or TabularData

Please let me know what you'd prefer.
 
> "Array of the heading contained in the file."
> 
> Now I'm really confused as to what a heading is. I thought it was the
> first line? Now there's an array of them?

The line did not say "headings," but I have change it to:

"Heading from the input in array form."
 
> No examples given for constructors.

I provide an example of using RecordList. Any example for the 
constructors would be the same or similar to the 7 other examples.

Is an example needed for both constructors, and maybe an example for 
front too?
 
> No documentation for front() or empty().

These are range primitives, those programming in D already know "returns 
the current element of the range." I find it more appropriate to describe 
what is returned by front() in the description of the range (many ranges 
have become private and have no documentation at all, not even their 
existence).

I will of course add documentation, but recommendations on non-pointless 
documentation would be welcome. Or how to handle the different types 
which can be assigned to Contents.

> "Record"
> 
> "Returned by a RecordList when Contents is a non-struct."
> 
> RecordList is a struct, it does not return anything.

It is a range, are we not allowed to talk of them as though they were 
high-level concepts? Do I leave off where you'd obtain a Record (it is 
private). Should I say "Returned by an instance of RecordList when 
calling front?"
 
> No examples.

It is private, removing doc comment for constructor.
 
> "csvNextToken"
> 
> Example?

Added.

Hopefully I addressed most of the documentation. Keep them coming.