View mode: basic / threaded / horizontal-split · Log in · Help
October 28, 2011
Review of Jesse Phillips's CSV Parser
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
October 28, 2011
Re: Review of Jesse Phillips's CSV Parser
On Fri, 28 Oct 2011 16:18:27 +0300, dsimcha <dsimcha@yahoo.com> wrote:

> Docs:
> http://nascent.freeshell.org/programming/D/doc/phobos/std_csv.html

Documentation should have examples of basic usage at the top. It should be  
possible to figure out how to use a CSV parser within 10 seconds of  
looking at the docs.

It looks like this is only a CSV parser, but not a CSV formatter? That's  
pretty weird. It'd be like std.xml or std.json only having reading  
functionality.

-- 
Best regards,
 Vladimir                            mailto:vladimir@thecybershadow.net
October 28, 2011
Re: Review of Jesse Phillips's CSV Parser
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

Could csvText!Struct(...) be generalized to also support Tuples and 
(eventually) classes?
October 28, 2011
Re: Review of Jesse Phillips's CSV Parser
On 10/28/2011 9: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

I'll kick this off with my review of the documentation (I'll review the 
implementation later):

"Header may be provided as first line in file" -> "A header may be 
provided as first line in file"?

csvText:  Shouldn't heading be a generic range of strings instead of a 
string[]?

Why should the order of the heading provided to csvText matter?  csvText 
should simply rearrange its results.

In csvNextToken:  "The expected use of this would be to create a parser. 
And may also be useful when handling errors within a CSV file."  Please 
re-word this sentence so it's grammatically correct.
October 28, 2011
Re: Review of Jesse Phillips's CSV Parser
On 2011-10-28 15:25, Piotr Szturmaj wrote:
> 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
>
> Could csvText!Struct(...) be generalized to also support Tuples and
> (eventually) classes?

Sounds more like a serialization library with a CSV archive type to me.

-- 
/Jacob Carlborg
October 28, 2011
Re: Review of Jesse Phillips's CSV Parser
On 2011-10-28 15:18, 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

If a header is specified, is it possible to iterate over the content as 
an associative array, something like this:

string str = "a,b,c\nHello,65,63.63\nWorld,123,3673.562";
auto records = csvText(str, ["b"]);

foreach (header, value ; records) {}

Or accessing a value by header:

foreach (record ; records)
{
    auto value = record["a"];
    auto val = record.b; // perhaps using opDispatch as well
}

-- 
/Jacob Carlborg
October 28, 2011
Re: Review of Jesse Phillips's CSV Parser
Jacob Carlborg wrote:
> On 2011-10-28 15:25, Piotr Szturmaj wrote:
>> 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
>>
>> Could csvText!Struct(...) be generalized to also support Tuples and
>> (eventually) classes?
>
> Sounds more like a serialization library with a CSV archive type to me.
>

I suppose you misunderstood my intentions. Here's an example from the docs:

string str = "Hello,65,63.63\nWorld,123,3673.562";
struct Layout {
    string name;
    int value;
    double other;
}

auto records = csvText!Layout(str);

foreach(record; records) {
    writeln(record.name);
    writeln(record.value);
    writeln(record.other);
}

I want to occasionally write:

alias Tuple!(string, int, double) Layout; // instead of struct

It should be easy to extend, but it would be nice if we have had general 
construct in Phobos. So instead of is(Contents == struct) @ 
https://github.com/he-the-great/phobos/blob/csv/std/csv.d#L433 there 
should be isComposite!Contents and FieldTypeTuple should be generalized 
(if it isn't) to structs, classes and tuples. I don't know if Tuple as a 
struct doesn't return valid fields from FieldTypeTuple now.
October 28, 2011
Re: Review of Jesse Phillips's CSV Parser
Le 28/10/2011 16:09, Piotr Szturmaj a écrit :
> Jacob Carlborg wrote:
>> On 2011-10-28 15:25, Piotr Szturmaj wrote:
>>> 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
>>>
>>> Could csvText!Struct(...) be generalized to also support Tuples and
>>> (eventually) classes?
>>
>> Sounds more like a serialization library with a CSV archive type to me.
>>
>
> I suppose you misunderstood my intentions. Here's an example from the docs:
>
> string str = "Hello,65,63.63\nWorld,123,3673.562";
> struct Layout {
> string name;
> int value;
> double other;
> }
>
> auto records = csvText!Layout(str);
>
> foreach(record; records) {
> writeln(record.name);
> writeln(record.value);
> writeln(record.other);
> }
>
> I want to occasionally write:
>
> alias Tuple!(string, int, double) Layout; // instead of struct
>
> It should be easy to extend, but it would be nice if we have had general
> construct in Phobos. So instead of is(Contents == struct) @
> https://github.com/he-the-great/phobos/blob/csv/std/csv.d#L433 there
> should be isComposite!Contents and FieldTypeTuple should be generalized
> (if it isn't) to structs, classes and tuples. I don't know if Tuple as a
> struct doesn't return valid fields from FieldTypeTuple now.

That would be great !
October 28, 2011
Re: Review of Jesse Phillips's CSV Parser
On 10/28/11 10: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

I like the idea of reading a CSV into a struct, or treating all fields 
as ints.

But, in my experience, CSVs are never perfect... because humans fill 
them, and humans aren't perfect or don't know "Oh, of course I must fill 
only numbers in this column, otherwise programs will explode".

So although it's a nice functionality, I think just returning everything 
as strings is more than enough.
October 28, 2011
Re: Review of Jesse Phillips's CSV Parser
dsimcha Wrote:

> I'll kick this off with my review of the documentation (I'll review the 
> implementation later):
> 
> "Header may be provided as first line in file" -> "A header may be 
> provided as first line in file"?
> 
> csvText:  Shouldn't heading be a generic range of strings instead of a 
> string[]?

I should revisit that. It will still need to be random access and possible some others, but doesn't need to be an array.

> Why should the order of the heading provided to csvText matter?  csvText 
> should simply rearrange its results.
> 
> In csvNextToken:  "The expected use of this would be to create a parser. 
> And may also be useful when handling errors within a CSV file."  Please 
> re-word this sentence so it's grammatically correct.
« First   ‹ Prev
1 2 3 4 5
Top | Discussion index | About this forum | D home