[CSV] Creating CSVParser instances

classic Classic list List threaded Threaded
30 messages Options
12
Reply | Threaded
Open this post in threaded view
|

[CSV] Creating CSVParser instances

Benedikt Ritter-4
Hi,

we currently have several static factory methods in CSVParser:

- public static CSVParser parseFile(File file, final CSVFormat format)
throws IOException
- public static CSVParser parseResource(String resource, Charset charset,
ClassLoader classLoader,
            final CSVFormat format) throws IOException
- public static CSVParser parseResource(String resource, Charset charset,
final CSVFormat format) throws IOException
- public static CSVParser parseString(String string) throws IOException
- public static CSVParser parseString(String string, final CSVFormat
format) throws IOException
- public static CSVParser parseURL(URL url, Charset charset, final
CSVFormat format) throws IOException

and one instance factory method in CSVFormat:

- public CSVParser parse(final Reader in) throws IOException

One can also create a parser using the public constructors defined in
CSVParser:

- public CSVParser(final Reader input) throws IOException
- public CSVParser(final Reader reader, final CSVFormat format) throws
IOException

I'm wondering:

1. do we need all this different ways to create CSVParsers? For example it
may be confusing to have parse(Reader) in CSVFormat which is pretty much
the same as CSVParser(Reader, CSVFormat) just the other way around.

2. all the factory methods are named "parseXXX" but they don't actually
parse anything. They just create an object that is capable of parsing CSV
content. Should the factory methods be renamed?

Benedikt


--
http://people.apache.org/~britter/
http://www.systemoutprintln.de/
http://twitter.com/BenediktRitter
http://github.com/britter
Reply | Threaded
Open this post in threaded view
|

Re: [CSV] Creating CSVParser instances

Emmanuel Bourg-3
Thank you for pointing this, I didn't notice this recent addition.

I'm not fond of the parseType(Type t) methods, I'd prefer parse(Type t).

Btw I would have kept these methods out of csv 1.0, this is again
delaying the release with another discussion...

Emmanuel Bourg


Le 08/08/2013 10:24, Benedikt Ritter a écrit :

> Hi,
>
> we currently have several static factory methods in CSVParser:
>
> - public static CSVParser parseFile(File file, final CSVFormat format)
> throws IOException
> - public static CSVParser parseResource(String resource, Charset charset,
> ClassLoader classLoader,
>             final CSVFormat format) throws IOException
> - public static CSVParser parseResource(String resource, Charset charset,
> final CSVFormat format) throws IOException
> - public static CSVParser parseString(String string) throws IOException
> - public static CSVParser parseString(String string, final CSVFormat
> format) throws IOException
> - public static CSVParser parseURL(URL url, Charset charset, final
> CSVFormat format) throws IOException
>
> and one instance factory method in CSVFormat:
>
> - public CSVParser parse(final Reader in) throws IOException
>
> One can also create a parser using the public constructors defined in
> CSVParser:
>
> - public CSVParser(final Reader input) throws IOException
> - public CSVParser(final Reader reader, final CSVFormat format) throws
> IOException
>
> I'm wondering:
>
> 1. do we need all this different ways to create CSVParsers? For example it
> may be confusing to have parse(Reader) in CSVFormat which is pretty much
> the same as CSVParser(Reader, CSVFormat) just the other way around.
>
> 2. all the factory methods are named "parseXXX" but they don't actually
> parse anything. They just create an object that is capable of parsing CSV
> content. Should the factory methods be renamed?
>
> Benedikt
>
>


---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: [CSV] Creating CSVParser instances

Benedikt Ritter-4
2013/8/8 Emmanuel Bourg <[hidden email]>

> Thank you for pointing this, I didn't notice this recent addition.
>
> I'm not fond of the parseType(Type t) methods, I'd prefer parse(Type t).
>

Different method names make sense for parseResource(String, Charset,
ClassLoader, CSVFormat) and parseString(String). One can see the difference
just from looking at it.
This is not true for parse(String, Charset, ClassLoader, CSVFormat)
compared to parse(String)

Anyway we would still have the problems I outline:
- the same CSVParser can be created in different ways (via contructor and
via CSVFormat.parse(Reader)).
- parse methods don't actually parse. They just create parser objects,
which may be confusing.


>
> Btw I would have kept these methods out of csv 1.0, this is again
> delaying the release with another discussion...


> Emmanuel Bourg
>
>
> Le 08/08/2013 10:24, Benedikt Ritter a écrit :
> > Hi,
> >
> > we currently have several static factory methods in CSVParser:
> >
> > - public static CSVParser parseFile(File file, final CSVFormat format)
> > throws IOException
> > - public static CSVParser parseResource(String resource, Charset charset,
> > ClassLoader classLoader,
> >             final CSVFormat format) throws IOException
> > - public static CSVParser parseResource(String resource, Charset charset,
> > final CSVFormat format) throws IOException
> > - public static CSVParser parseString(String string) throws IOException
> > - public static CSVParser parseString(String string, final CSVFormat
> > format) throws IOException
> > - public static CSVParser parseURL(URL url, Charset charset, final
> > CSVFormat format) throws IOException
> >
> > and one instance factory method in CSVFormat:
> >
> > - public CSVParser parse(final Reader in) throws IOException
> >
> > One can also create a parser using the public constructors defined in
> > CSVParser:
> >
> > - public CSVParser(final Reader input) throws IOException
> > - public CSVParser(final Reader reader, final CSVFormat format) throws
> > IOException
> >
> > I'm wondering:
> >
> > 1. do we need all this different ways to create CSVParsers? For example
> it
> > may be confusing to have parse(Reader) in CSVFormat which is pretty much
> > the same as CSVParser(Reader, CSVFormat) just the other way around.
> >
> > 2. all the factory methods are named "parseXXX" but they don't actually
> > parse anything. They just create an object that is capable of parsing CSV
> > content. Should the factory methods be renamed?
> >
> > Benedikt
> >
> >
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: [hidden email]
> For additional commands, e-mail: [hidden email]
>
>


--
http://people.apache.org/~britter/
http://www.systemoutprintln.de/
http://twitter.com/BenediktRitter
http://github.com/britter
Reply | Threaded
Open this post in threaded view
|

Re: [CSV] Creating CSVParser instances

Emmanuel Bourg-3
Le 08/08/2013 14:25, Benedikt Ritter a écrit :

> Anyway we would still have the problems I outline:
> - the same CSVParser can be created in different ways (via contructor and
> via CSVFormat.parse(Reader)).

And from CSVFormat.parse()

> - parse methods don't actually parse. They just create parser objects,
> which may be confusing.

'parse' isn't a bad name, considering the idiom using a foreach loop:

for (CSVRecord record : CSVParser.parse(input)) {
    ....
}

But that doesn't play well with the parse methods using a File or an
URL, because the underlying reader can't be closed. So I'd rather keep
the resource management out of CSVParser.

Emmanuel Bourg


---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: [CSV] Creating CSVParser instances

garydgregory
In reply to this post by Benedikt Ritter-4
On Thu, Aug 8, 2013 at 4:24 AM, Benedikt Ritter <[hidden email]> wrote:

> Hi,
>
> we currently have several static factory methods in CSVParser:
>
> - public static CSVParser parseFile(File file, final CSVFormat format)
> throws IOException
> - public static CSVParser parseResource(String resource, Charset charset,
> ClassLoader classLoader,
>             final CSVFormat format) throws IOException
> - public static CSVParser parseResource(String resource, Charset charset,
> final CSVFormat format) throws IOException
> - public static CSVParser parseString(String string) throws IOException
> - public static CSVParser parseString(String string, final CSVFormat
> format) throws IOException
> - public static CSVParser parseURL(URL url, Charset charset, final
> CSVFormat format) throws IOException
>
> and one instance factory method in CSVFormat:
>
> - public CSVParser parse(final Reader in) throws IOException
>
> One can also create a parser using the public constructors defined in
> CSVParser:
>
> - public CSVParser(final Reader input) throws IOException
> - public CSVParser(final Reader reader, final CSVFormat format) throws
> IOException
>
> I'm wondering:
>
> 1. do we need all this different ways to create CSVParsers? For example it
> may be confusing to have parse(Reader) in CSVFormat which is pretty much
> the same as CSVParser(Reader, CSVFormat) just the other way around.
>
> 2. all the factory methods are named "parseXXX" but they don't actually
> parse anything. They just create an object that is capable of parsing CSV
> content. Should the factory methods be renamed?
>

Here is how I see it:

The one instance factory method in CSVFormat is a convenience that I do not
find useful, it just makes for a nice demo _in theory_. In my experience, I
always create utility methods to create Readers from different sources. I
would be OK removing this method in favor of centralized construction in
CSVParser or a new  CSVParseFactory.

If we had a CSVParseFactory, we could remove parsing in CSVFormat and make
the CSVParser ctors package private.

The naming pattern for factory method could also be "createParser" or
"create[Type]Parser". The only time the type name is really needed IMO is
to distinguish a resource string, from CSV content. I am assuming that a
file would be passed in as a File, instead of a file name.

Gary


> Benedikt
>
>
> --
> http://people.apache.org/~britter/
> http://www.systemoutprintln.de/
> http://twitter.com/BenediktRitter
> http://github.com/britter
>



--
E-Mail: [hidden email] | [hidden email]
Java Persistence with Hibernate, Second Edition<http://www.manning.com/bauer3/>
JUnit in Action, Second Edition <http://www.manning.com/tahchiev/>
Spring Batch in Action <http://www.manning.com/templier/>
Blog: http://garygregory.wordpress.com
Home: http://garygregory.com/
Tweet! http://twitter.com/GaryGregory
Reply | Threaded
Open this post in threaded view
|

Re: [CSV] Creating CSVParser instances

garydgregory
In reply to this post by Emmanuel Bourg-3
On Thu, Aug 8, 2013 at 5:30 AM, Emmanuel Bourg <[hidden email]> wrote:

> Thank you for pointing this, I didn't notice this recent addition.
>
> I'm not fond of the parseType(Type t) methods, I'd prefer parse(Type t).
>

I'm OK with 'parse' or 'createParser'. WRT to types, the only time the type
name is really needed IMO is to distinguish a resource string, from CSV
content. I am assuming that a file would be passed in as a File, instead of
a file name. I also like consistency, so it's nice to have the type names
in all or none. To distinguish a resource path string from CSV content, if
the API has a class loader arg, then the string should be a resource path,
so we could remove all type names from the method names. I'll experiment
with that...

Gary


>
> Btw I would have kept these methods out of csv 1.0, this is again
> delaying the release with another discussion...
>
> Emmanuel Bourg
>
>
> Le 08/08/2013 10:24, Benedikt Ritter a écrit :
> > Hi,
> >
> > we currently have several static factory methods in CSVParser:
> >
> > - public static CSVParser parseFile(File file, final CSVFormat format)
> > throws IOException
> > - public static CSVParser parseResource(String resource, Charset charset,
> > ClassLoader classLoader,
> >             final CSVFormat format) throws IOException
> > - public static CSVParser parseResource(String resource, Charset charset,
> > final CSVFormat format) throws IOException
> > - public static CSVParser parseString(String string) throws IOException
> > - public static CSVParser parseString(String string, final CSVFormat
> > format) throws IOException
> > - public static CSVParser parseURL(URL url, Charset charset, final
> > CSVFormat format) throws IOException
> >
> > and one instance factory method in CSVFormat:
> >
> > - public CSVParser parse(final Reader in) throws IOException
> >
> > One can also create a parser using the public constructors defined in
> > CSVParser:
> >
> > - public CSVParser(final Reader input) throws IOException
> > - public CSVParser(final Reader reader, final CSVFormat format) throws
> > IOException
> >
> > I'm wondering:
> >
> > 1. do we need all this different ways to create CSVParsers? For example
> it
> > may be confusing to have parse(Reader) in CSVFormat which is pretty much
> > the same as CSVParser(Reader, CSVFormat) just the other way around.
> >
> > 2. all the factory methods are named "parseXXX" but they don't actually
> > parse anything. They just create an object that is capable of parsing CSV
> > content. Should the factory methods be renamed?
> >
> > Benedikt
> >
> >
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: [hidden email]
> For additional commands, e-mail: [hidden email]
>
>


--
E-Mail: [hidden email] | [hidden email]
Java Persistence with Hibernate, Second Edition<http://www.manning.com/bauer3/>
JUnit in Action, Second Edition <http://www.manning.com/tahchiev/>
Spring Batch in Action <http://www.manning.com/templier/>
Blog: http://garygregory.wordpress.com
Home: http://garygregory.com/
Tweet! http://twitter.com/GaryGregory
Reply | Threaded
Open this post in threaded view
|

Re: [CSV] Creating CSVParser instances

garydgregory
In reply to this post by garydgregory
On Thu, Aug 8, 2013 at 9:10 AM, Gary Gregory <[hidden email]> wrote:

> On Thu, Aug 8, 2013 at 4:24 AM, Benedikt Ritter <[hidden email]>wrote:
>
>> Hi,
>>
>> we currently have several static factory methods in CSVParser:
>>
>> - public static CSVParser parseFile(File file, final CSVFormat format)
>> throws IOException
>> - public static CSVParser parseResource(String resource, Charset charset,
>> ClassLoader classLoader,
>>             final CSVFormat format) throws IOException
>> - public static CSVParser parseResource(String resource, Charset charset,
>> final CSVFormat format) throws IOException
>> - public static CSVParser parseString(String string) throws IOException
>> - public static CSVParser parseString(String string, final CSVFormat
>> format) throws IOException
>> - public static CSVParser parseURL(URL url, Charset charset, final
>> CSVFormat format) throws IOException
>>
>> and one instance factory method in CSVFormat:
>>
>> - public CSVParser parse(final Reader in) throws IOException
>>
>> One can also create a parser using the public constructors defined in
>> CSVParser:
>>
>> - public CSVParser(final Reader input) throws IOException
>> - public CSVParser(final Reader reader, final CSVFormat format) throws
>> IOException
>>
>> I'm wondering:
>>
>> 1. do we need all this different ways to create CSVParsers? For example it
>> may be confusing to have parse(Reader) in CSVFormat which is pretty much
>> the same as CSVParser(Reader, CSVFormat) just the other way around.
>>
>> 2. all the factory methods are named "parseXXX" but they don't actually
>> parse anything. They just create an object that is capable of parsing CSV
>> content. Should the factory methods be renamed?
>>
>
> Here is how I see it:
>
> The one instance factory method in CSVFormat is a convenience that I do
> not find useful, it just makes for a nice demo _in theory_. In my
> experience, I always create utility methods to create Readers from
> different sources. I would be OK removing this method in favor of
> centralized construction in CSVParser or a new  CSVParseFactory.
>

Well, there is no need for a CSVParseFactory of course, the CSVParse ctors
can be private and the same feature (Reader in) can be in a factory method.
This gives us the freedom to change the parser ctors later.

Gary

>
> If we had a CSVParseFactory, we could remove parsing in CSVFormat and make
> the CSVParser ctors package private.
>
> The naming pattern for factory method could also be "createParser" or
> "create[Type]Parser". The only time the type name is really needed IMO is
> to distinguish a resource string, from CSV content. I am assuming that a
> file would be passed in as a File, instead of a file name.
>
> Gary
>
>
>> Benedikt
>>
>>
>> --
>> http://people.apache.org/~britter/
>> http://www.systemoutprintln.de/
>> http://twitter.com/BenediktRitter
>> http://github.com/britter
>>
>
>
>
> --
> E-Mail: [hidden email] | [hidden email]
> Java Persistence with Hibernate, Second Edition<http://www.manning.com/bauer3/>
> JUnit in Action, Second Edition <http://www.manning.com/tahchiev/>
> Spring Batch in Action <http://www.manning.com/templier/>
> Blog: http://garygregory.wordpress.com
> Home: http://garygregory.com/
> Tweet! http://twitter.com/GaryGregory
>



--
E-Mail: [hidden email] | [hidden email]
Java Persistence with Hibernate, Second Edition<http://www.manning.com/bauer3/>
JUnit in Action, Second Edition <http://www.manning.com/tahchiev/>
Spring Batch in Action <http://www.manning.com/templier/>
Blog: http://garygregory.wordpress.com
Home: http://garygregory.com/
Tweet! http://twitter.com/GaryGregory
Reply | Threaded
Open this post in threaded view
|

Re: [CSV] Creating CSVParser instances

garydgregory
In reply to this post by garydgregory
On Thu, Aug 8, 2013 at 9:14 AM, Gary Gregory <[hidden email]> wrote:

> On Thu, Aug 8, 2013 at 5:30 AM, Emmanuel Bourg <[hidden email]> wrote:
>
>> Thank you for pointing this, I didn't notice this recent addition.
>>
>> I'm not fond of the parseType(Type t) methods, I'd prefer parse(Type t).
>>
>
> I'm OK with 'parse' or 'createParser'. WRT to types, the only time the
> type name is really needed IMO is to distinguish a resource string, from
> CSV content. I am assuming that a file would be passed in as a File,
> instead of a file name. I also like consistency, so it's nice to have the
> type names in all or none. To distinguish a resource path string from CSV
> content, if the API has a class loader arg, then the string should be a
> resource path, so we could remove all type names from the method names.
> I'll experiment with that...
>

With types names:

parseFile(File, CSVFormat)
parseResource(String, Charset, ClassLoader, CSVFormat)
parseResource(String, Charset, CSVFormat)
[parseString(String)]
parseString(String, CSVFormat)
parseURL(URL, Charset, CSVFormat)

Sans type names:

parse(File, CSVFormat)
parse(String, Charset, ClassLoader, CSVFormat)
parse(String, Charset, CSVFormat)
[parse(String)]
parse(String, CSVFormat)
parse(URL, Charset, CSVFormat)

[I'd probably remove parse(String) so that all APIs take a CSVFormat.]

Gary


>
> Gary
>
>
>>
>> Btw I would have kept these methods out of csv 1.0, this is again
>> delaying the release with another discussion...
>>
>> Emmanuel Bourg
>>
>>
>> Le 08/08/2013 10:24, Benedikt Ritter a écrit :
>> > Hi,
>> >
>> > we currently have several static factory methods in CSVParser:
>> >
>> > - public static CSVParser parseFile(File file, final CSVFormat format)
>> > throws IOException
>> > - public static CSVParser parseResource(String resource, Charset
>> charset,
>> > ClassLoader classLoader,
>> >             final CSVFormat format) throws IOException
>> > - public static CSVParser parseResource(String resource, Charset
>> charset,
>> > final CSVFormat format) throws IOException
>> > - public static CSVParser parseString(String string) throws IOException
>> > - public static CSVParser parseString(String string, final CSVFormat
>> > format) throws IOException
>> > - public static CSVParser parseURL(URL url, Charset charset, final
>> > CSVFormat format) throws IOException
>> >
>> > and one instance factory method in CSVFormat:
>> >
>> > - public CSVParser parse(final Reader in) throws IOException
>> >
>> > One can also create a parser using the public constructors defined in
>> > CSVParser:
>> >
>> > - public CSVParser(final Reader input) throws IOException
>> > - public CSVParser(final Reader reader, final CSVFormat format) throws
>> > IOException
>> >
>> > I'm wondering:
>> >
>> > 1. do we need all this different ways to create CSVParsers? For example
>> it
>> > may be confusing to have parse(Reader) in CSVFormat which is pretty much
>> > the same as CSVParser(Reader, CSVFormat) just the other way around.
>> >
>> > 2. all the factory methods are named "parseXXX" but they don't actually
>> > parse anything. They just create an object that is capable of parsing
>> CSV
>> > content. Should the factory methods be renamed?
>> >
>> > Benedikt
>> >
>> >
>>
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: [hidden email]
>> For additional commands, e-mail: [hidden email]
>>
>>
>
>
> --
> E-Mail: [hidden email] | [hidden email]
> Java Persistence with Hibernate, Second Edition<http://www.manning.com/bauer3/>
> JUnit in Action, Second Edition <http://www.manning.com/tahchiev/>
> Spring Batch in Action <http://www.manning.com/templier/>
> Blog: http://garygregory.wordpress.com
> Home: http://garygregory.com/
> Tweet! http://twitter.com/GaryGregory
>



--
E-Mail: [hidden email] | [hidden email]
Java Persistence with Hibernate, Second Edition<http://www.manning.com/bauer3/>
JUnit in Action, Second Edition <http://www.manning.com/tahchiev/>
Spring Batch in Action <http://www.manning.com/templier/>
Blog: http://garygregory.wordpress.com
Home: http://garygregory.com/
Tweet! http://twitter.com/GaryGregory
Reply | Threaded
Open this post in threaded view
|

Re: [CSV] Creating CSVParser instances

Emmanuel Bourg-3
In reply to this post by garydgregory
Le 08/08/2013 15:10, Gary Gregory a écrit :

> The one instance factory method in CSVFormat is a convenience that I do not
> find useful, it just makes for a nice demo _in theory_. In my experience, I
> always create utility methods to create Readers from different sources. I
> would be OK removing this method in favor of centralized construction in
> CSVParser or a new  CSVParseFactory.
>
> If we had a CSVParseFactory, we could remove parsing in CSVFormat and make
> the CSVParser ctors package private.

Please don't remove CSVFormat.parse(). I disagree with the "demo"
effect, that's how I use the API everyday.

Emmanuel Bourg



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: [CSV] Creating CSVParser instances

Emmanuel Bourg-3
In reply to this post by garydgregory
Le 08/08/2013 15:40, Gary Gregory a écrit :

> Sans type names:
>
> parse(File, CSVFormat)
> parse(String, Charset, ClassLoader, CSVFormat)
> parse(String, Charset, CSVFormat)
> [parse(String)]
> parse(String, CSVFormat)
> parse(URL, Charset, CSVFormat)

That looks better. I would remove the methods for a classpath resource,
that's a less common case. That would make:

parse(File, CSVFormat)
parse(String, CSVFormat)
parse(URL, Charset, CSVFormat)

And you probably want a charset for the File too.


> [I'd probably remove parse(String) so that all APIs take a CSVFormat.]

+1.

And at this point you realize they could belong to CSVFormat, because
they all need one to operate.

    format.parse(file):

instead of:

    CSVParser.parse(file, format);


Emmanuel Bourg


---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: [CSV] Creating CSVParser instances

Benedikt Ritter-4
2013/8/8 Emmanuel Bourg <[hidden email]>

> Le 08/08/2013 15:40, Gary Gregory a écrit :
>
> > Sans type names:
> >
> > parse(File, CSVFormat)
> > parse(String, Charset, ClassLoader, CSVFormat)
> > parse(String, Charset, CSVFormat)
> > [parse(String)]
> > parse(String, CSVFormat)
> > parse(URL, Charset, CSVFormat)
>
> That looks better. I would remove the methods for a classpath resource,
> that's a less common case. That would make:
>
> parse(File, CSVFormat)
> parse(String, CSVFormat)
> parse(URL, Charset, CSVFormat)
>
> And you probably want a charset for the File too.
>
>
> > [I'd probably remove parse(String) so that all APIs take a CSVFormat.]
>
> +1.
>
> And at this point you realize they could belong to CSVFormat, because
> they all need one to operate.
>
>     format.parse(file):
>

A format can parse something... That sounds strange to me.
Let's rename it to

   format.createParser(file)

I'm +1 for having only one place to create parsers.
And having less parameters is (in most cases) better.


>
> instead of:
>
>     CSVParser.parse(file, format);
>
>
> Emmanuel Bourg
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: [hidden email]
> For additional commands, e-mail: [hidden email]
>
>


--
http://people.apache.org/~britter/
http://www.systemoutprintln.de/
http://twitter.com/BenediktRitter
http://github.com/britter
Reply | Threaded
Open this post in threaded view
|

Re: [CSV] Creating CSVParser instances

Emmanuel Bourg-3
Le 08/08/2013 16:27, Benedikt Ritter a écrit :

> A format can parse something... That sounds strange to me.

That's pretty common though. NumberFormat, DateFormat are good examples.


> Let's rename it to
>
>    format.createParser(file)

Please, no :)


Emmanuel


---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: [CSV] Creating CSVParser instances

Benedikt Ritter-4
2013/8/8 Emmanuel Bourg <[hidden email]>

> Le 08/08/2013 16:27, Benedikt Ritter a écrit :
>
> > A format can parse something... That sounds strange to me.
>
> That's pretty common though. NumberFormat, DateFormat are good examples.
>
>
> > Let's rename it to
> >
> >    format.createParser(file)
>
> Please, no :)
>

Damn it, Emmanuel is always against me ;-)

Okay then: leave it as it is, but move factory methods from CSVParser to
CSVFormat?
CSVFormat will be the only place to create CSVParsers.


>
>
> Emmanuel
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: [hidden email]
> For additional commands, e-mail: [hidden email]
>
>


--
http://people.apache.org/~britter/
http://www.systemoutprintln.de/
http://twitter.com/BenediktRitter
http://github.com/britter
Reply | Threaded
Open this post in threaded view
|

Re: [CSV] Creating CSVParser instances

Adrian Crum-3
In reply to this post by garydgregory
+1 for createParser(...)

-Adrian

On 8/8/2013 6:14 AM, Gary Gregory wrote:

> On Thu, Aug 8, 2013 at 5:30 AM, Emmanuel Bourg <[hidden email]> wrote:
>
>> Thank you for pointing this, I didn't notice this recent addition.
>>
>> I'm not fond of the parseType(Type t) methods, I'd prefer parse(Type t).
>>
> I'm OK with 'parse' or 'createParser'. WRT to types, the only time the type
> name is really needed IMO is to distinguish a resource string, from CSV
> content. I am assuming that a file would be passed in as a File, instead of
> a file name. I also like consistency, so it's nice to have the type names
> in all or none. To distinguish a resource path string from CSV content, if
> the API has a class loader arg, then the string should be a resource path,
> so we could remove all type names from the method names. I'll experiment
> with that...
>
> Gary
>
>
>> Btw I would have kept these methods out of csv 1.0, this is again
>> delaying the release with another discussion...
>>
>> Emmanuel Bourg
>>
>>
>> Le 08/08/2013 10:24, Benedikt Ritter a écrit :
>>> Hi,
>>>
>>> we currently have several static factory methods in CSVParser:
>>>
>>> - public static CSVParser parseFile(File file, final CSVFormat format)
>>> throws IOException
>>> - public static CSVParser parseResource(String resource, Charset charset,
>>> ClassLoader classLoader,
>>>              final CSVFormat format) throws IOException
>>> - public static CSVParser parseResource(String resource, Charset charset,
>>> final CSVFormat format) throws IOException
>>> - public static CSVParser parseString(String string) throws IOException
>>> - public static CSVParser parseString(String string, final CSVFormat
>>> format) throws IOException
>>> - public static CSVParser parseURL(URL url, Charset charset, final
>>> CSVFormat format) throws IOException
>>>
>>> and one instance factory method in CSVFormat:
>>>
>>> - public CSVParser parse(final Reader in) throws IOException
>>>
>>> One can also create a parser using the public constructors defined in
>>> CSVParser:
>>>
>>> - public CSVParser(final Reader input) throws IOException
>>> - public CSVParser(final Reader reader, final CSVFormat format) throws
>>> IOException
>>>
>>> I'm wondering:
>>>
>>> 1. do we need all this different ways to create CSVParsers? For example
>> it
>>> may be confusing to have parse(Reader) in CSVFormat which is pretty much
>>> the same as CSVParser(Reader, CSVFormat) just the other way around.
>>>
>>> 2. all the factory methods are named "parseXXX" but they don't actually
>>> parse anything. They just create an object that is capable of parsing CSV
>>> content. Should the factory methods be renamed?
>>>
>>> Benedikt
>>>
>>>
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: [hidden email]
>> For additional commands, e-mail: [hidden email]
>>
>>
>


---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: [CSV] Creating CSVParser instances

Emmanuel Bourg-3
In reply to this post by Benedikt Ritter-4
Le 08/08/2013 16:43, Benedikt Ritter a écrit :

> Damn it, Emmanuel is always against me ;-)

I love everyone :) I'm just against unnecessary complexity.


> Okay then: leave it as it is, but move factory methods from CSVParser to
> CSVFormat?

I'd support that. It's no longer factory methods at this point though.


> CSVFormat will be the only place to create CSVParsers.

Plus the constructor of CSVParser of course.


Emmanuel Bourg


---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: [CSV] Creating CSVParser instances

Bernd Eckenfels
In reply to this post by Emmanuel Bourg-3
Am 08.08.2013, 16:09 Uhr, schrieb Emmanuel Bourg <[hidden email]>:

> Le 08/08/2013 15:40, Gary Gregory a écrit :
>> Sans type names:
>> parse(File, CSVFormat)
>> parse(String, Charset, ClassLoader, CSVFormat)
>> parse(String, Charset, CSVFormat)
>> [parse(String)]
>> parse(String, CSVFormat)
>> parse(URL, Charset, CSVFormat)
>
> That looks better. I would remove the methods for a classpath resource,
> that's a less common case. That would make:
>
> parse(File, CSVFormat)
> parse(String, CSVFormat)
> parse(URL, Charset, CSVFormat)

I think (InputStream, Charset) and (Reader) are the most important generic  
types. (Allowing also to adopt to CharSequence or Buffers).

Gruss
Bernd
--
http://www.zusammenkunft.net

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: [CSV] Creating CSVParser instances

garydgregory
In reply to this post by Emmanuel Bourg-3
On Thu, Aug 8, 2013 at 10:09 AM, Emmanuel Bourg <[hidden email]> wrote:

> Le 08/08/2013 15:40, Gary Gregory a écrit :
>
> > Sans type names:
> >
> > parse(File, CSVFormat)
> > parse(String, Charset, ClassLoader, CSVFormat)
> > parse(String, Charset, CSVFormat)
> > [parse(String)]
> > parse(String, CSVFormat)
> > parse(URL, Charset, CSVFormat)
>
> That looks better. I would remove the methods for a classpath resource,
> that's a less common case.


That's the data source _my_ app uses everyday ;)

Gary


> That would make:
>
> parse(File, CSVFormat)
> parse(String, CSVFormat)
> parse(URL, Charset, CSVFormat)
>
> And you probably want a charset for the File too.
>
>
> > [I'd probably remove parse(String) so that all APIs take a CSVFormat.]
>
> +1.
>
> And at this point you realize they could belong to CSVFormat, because
> they all need one to operate.
>
>     format.parse(file):
>
> instead of:
>
>     CSVParser.parse(file, format);
>
>
> Emmanuel Bourg
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: [hidden email]
> For additional commands, e-mail: [hidden email]
>
>


--
E-Mail: [hidden email] | [hidden email]
Java Persistence with Hibernate, Second Edition<http://www.manning.com/bauer3/>
JUnit in Action, Second Edition <http://www.manning.com/tahchiev/>
Spring Batch in Action <http://www.manning.com/templier/>
Blog: http://garygregory.wordpress.com
Home: http://garygregory.com/
Tweet! http://twitter.com/GaryGregory
Reply | Threaded
Open this post in threaded view
|

Re: [CSV] Creating CSVParser instances

garydgregory
In reply to this post by Emmanuel Bourg-3
On Thu, Aug 8, 2013 at 10:09 AM, Emmanuel Bourg <[hidden email]> wrote:

> Le 08/08/2013 15:40, Gary Gregory a écrit :
>
> > Sans type names:
> >
> > parse(File, CSVFormat)
> > parse(String, Charset, ClassLoader, CSVFormat)
> > parse(String, Charset, CSVFormat)
> > [parse(String)]
> > parse(String, CSVFormat)
> > parse(URL, Charset, CSVFormat)
>
> That looks better. I would remove the methods for a classpath resource,
> that's a less common case. That would make:
>
> parse(File, CSVFormat)
> parse(String, CSVFormat)
> parse(URL, Charset, CSVFormat)
>
> And you probably want a charset for the File too.
>
>
> > [I'd probably remove parse(String) so that all APIs take a CSVFormat.]
>
> +1.
>

OK, a wee bit of clean up then; now in SVN:

parse(File, CSVFormat)
parse(String, Charset, ClassLoader, CSVFormat)
parse(String, Charset, CSVFormat)
parse(String, CSVFormat)
parse(URL, Charset, CSVFormat)

Gary


>
> And at this point you realize they could belong to CSVFormat, because
> they all need one to operate.
>
>     format.parse(file):
>
> instead of:
>
>     CSVParser.parse(file, format);
>
>
> Emmanuel Bourg
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: [hidden email]
> For additional commands, e-mail: [hidden email]
>
>


--
E-Mail: [hidden email] | [hidden email]
Java Persistence with Hibernate, Second Edition<http://www.manning.com/bauer3/>
JUnit in Action, Second Edition <http://www.manning.com/tahchiev/>
Spring Batch in Action <http://www.manning.com/templier/>
Blog: http://garygregory.wordpress.com
Home: http://garygregory.com/
Tweet! http://twitter.com/GaryGregory
Reply | Threaded
Open this post in threaded view
|

Re: [CSV] Creating CSVParser instances

garydgregory
In reply to this post by Benedikt Ritter-4
On Thu, Aug 8, 2013 at 10:27 AM, Benedikt Ritter <[hidden email]> wrote:

> 2013/8/8 Emmanuel Bourg <[hidden email]>
>
> > Le 08/08/2013 15:40, Gary Gregory a écrit :
> >
> > > Sans type names:
> > >
> > > parse(File, CSVFormat)
> > > parse(String, Charset, ClassLoader, CSVFormat)
> > > parse(String, Charset, CSVFormat)
> > > [parse(String)]
> > > parse(String, CSVFormat)
> > > parse(URL, Charset, CSVFormat)
> >
> > That looks better. I would remove the methods for a classpath resource,
> > that's a less common case. That would make:
> >
> > parse(File, CSVFormat)
> > parse(String, CSVFormat)
> > parse(URL, Charset, CSVFormat)
> >
> > And you probably want a charset for the File too.
> >
> >
> > > [I'd probably remove parse(String) so that all APIs take a CSVFormat.]
> >
> > +1.
> >
> > And at this point you realize they could belong to CSVFormat, because
> > they all need one to operate.
> >
> >     format.parse(file):
> >
>
> A format can parse something... That sounds strange to me.
>

Same here, it sounds strange that a format does anything like parsing. A
parser parses, that's obvious. When I leave [csv] alone for a while and get
back to it, the parser is always where I go look for an API to get started.
It's just weird to start with a format IMO. I would be OK with leaving the
format parser API there I suppose, but I do not think the format should be
the kitchen sink for all other input sources. Well, you can try to convince
me of course ;)

Gary



> Let's rename it to
>
>    format.createParser(file)
>
> I'm +1 for having only one place to create parsers.
> And having less parameters is (in most cases) better.
>
>
> >
> > instead of:
> >
> >     CSVParser.parse(file, format);
> >
> >
> > Emmanuel Bourg
> >
> >
> > ---------------------------------------------------------------------
> > To unsubscribe, e-mail: [hidden email]
> > For additional commands, e-mail: [hidden email]
> >
> >
>
>
> --
> http://people.apache.org/~britter/
> http://www.systemoutprintln.de/
> http://twitter.com/BenediktRitter
> http://github.com/britter
>



--
E-Mail: [hidden email] | [hidden email]
Java Persistence with Hibernate, Second Edition<http://www.manning.com/bauer3/>
JUnit in Action, Second Edition <http://www.manning.com/tahchiev/>
Spring Batch in Action <http://www.manning.com/templier/>
Blog: http://garygregory.wordpress.com
Home: http://garygregory.com/
Tweet! http://twitter.com/GaryGregory
Reply | Threaded
Open this post in threaded view
|

Re: [CSV] Creating CSVParser instances

Benedikt Ritter-4
2013/8/8 Gary Gregory <[hidden email]>

> On Thu, Aug 8, 2013 at 10:27 AM, Benedikt Ritter <[hidden email]>
> wrote:
>
> > 2013/8/8 Emmanuel Bourg <[hidden email]>
> >
> > > Le 08/08/2013 15:40, Gary Gregory a écrit :
> > >
> > > > Sans type names:
> > > >
> > > > parse(File, CSVFormat)
> > > > parse(String, Charset, ClassLoader, CSVFormat)
> > > > parse(String, Charset, CSVFormat)
> > > > [parse(String)]
> > > > parse(String, CSVFormat)
> > > > parse(URL, Charset, CSVFormat)
> > >
> > > That looks better. I would remove the methods for a classpath resource,
> > > that's a less common case. That would make:
> > >
> > > parse(File, CSVFormat)
> > > parse(String, CSVFormat)
> > > parse(URL, Charset, CSVFormat)
> > >
> > > And you probably want a charset for the File too.
> > >
> > >
> > > > [I'd probably remove parse(String) so that all APIs take a
> CSVFormat.]
> > >
> > > +1.
> > >
> > > And at this point you realize they could belong to CSVFormat, because
> > > they all need one to operate.
> > >
> > >     format.parse(file):
> > >
> >
> > A format can parse something... That sounds strange to me.
> >
>
> Same here, it sounds strange that a format does anything like parsing. A
> parser parses, that's obvious. When I leave [csv] alone for a while and get
> back to it, the parser is always where I go look for an API to get started.
> It's just weird to start with a format IMO. I would be OK with leaving the
> format parser API there I suppose, but I do not think the format should be
> the kitchen sink for all other input sources. Well, you can try to convince
> me of course ;)
>

Okay, so do we want to let parse(Reader) method as a convenience in
CSVFormat?
The CSVParser will serve as the main entry point to the API.

What about the parse resource methods in CSVParser? Emmanuel has expressed
feelings against this addition. I personally think reading from the class
path should be done in client code. But since Gary seems to have a use case
for this and I cannot really judge how common it is to read csv data from
the class path I could live with this.

What I really don't like is:
   public static CSVParser parse(String, CSVFormat)
   public static CSVParser parse(String, CharSet, CSVFormat)

They look nearly the same yet they do completely different things... IMHO
this cannot stay this way.

WDYT?

Benedikt


>
> Gary
>
>
>
> > Let's rename it to
> >
> >    format.createParser(file)
> >
> > I'm +1 for having only one place to create parsers.
> > And having less parameters is (in most cases) better.
> >
> >
> > >
> > > instead of:
> > >
> > >     CSVParser.parse(file, format);
> > >
> > >
> > > Emmanuel Bourg
> > >
> > >
> > > ---------------------------------------------------------------------
> > > To unsubscribe, e-mail: [hidden email]
> > > For additional commands, e-mail: [hidden email]
> > >
> > >
> >
> >
> > --
> > http://people.apache.org/~britter/
> > http://www.systemoutprintln.de/
> > http://twitter.com/BenediktRitter
> > http://github.com/britter
> >
>
>
>
> --
> E-Mail: [hidden email] | [hidden email]
> Java Persistence with Hibernate, Second Edition<
> http://www.manning.com/bauer3/>
> JUnit in Action, Second Edition <http://www.manning.com/tahchiev/>
> Spring Batch in Action <http://www.manning.com/templier/>
> Blog: http://garygregory.wordpress.com
> Home: http://garygregory.com/
> Tweet! http://twitter.com/GaryGregory
>



--
http://people.apache.org/~britter/
http://www.systemoutprintln.de/
http://twitter.com/BenediktRitter
http://github.com/britter
12