commons-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From sebb <seb...@gmail.com>
Subject Re: [CSV] Creating CSVParser instances
Date Wed, 14 Aug 2013 07:54:29 GMT
On 14 August 2013 07:44, Benedikt Ritter <britter@apache.org> wrote:
> 2013/8/12 Gary Gregory <garydgregory@gmail.com>
>
>> On Mon, Aug 12, 2013 at 3:13 PM, Benedikt Ritter <britter@apache.org>
>> wrote:
>>
>> > 2013/8/8 Gary Gregory <garydgregory@gmail.com>
>> >
>> > > On Thu, Aug 8, 2013 at 10:27 AM, Benedikt Ritter <britter@apache.org>
>> > > wrote:
>> > >
>> > > > 2013/8/8 Emmanuel Bourg <ebourg@apache.org>
>> > > >
>> > > > > 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?
>> >
>>
>> Then it is back to "parseResource"  or removing it altogether. Either way,
>> I'll live with ;)
>>
>
> Hey Gary,
>
> I know that reading from classpath is a use case for your app.
> But to be honest I feel that this doesn't fit into the parser.
> Fiddling around with the classpath should be done in client code.

Agreed.

> Would it be okay for you if we remove those methods?

I had not noticed the classpath methods otherwise I would have
objected earlier to including them.

> that would leave us with:
>
> public static CSVParser parse(File file, final CSVFormat format) throws
> IOException
> public static CSVParser parse(String string, final CSVFormat format) throws
> IOException
> public static CSVParser parse(URL url, Charset charset, final CSVFormat
> format) throws IOException
>
> public CSVParser(final Reader input) throws IOException
> public CSVParser(final Reader reader, final CSVFormat format) throws
> IOException
>
> Looks clear to me. Only minor nit is that we only have parser methods that
> take a CSVFormat but we have a constructor with and without. We can
> probably remove the one that only takes a reader.

Agreed. It's not necessary.

> Benedikt
>
>
>>
>> Gary
>>
>>
>> >
>> > 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: dev-unsubscribe@commons.apache.org
>> > > > > For additional commands, e-mail: dev-help@commons.apache.org
>> > > > >
>> > > > >
>> > > >
>> > > >
>> > > > --
>> > > > http://people.apache.org/~britter/
>> > > > http://www.systemoutprintln.de/
>> > > > http://twitter.com/BenediktRitter
>> > > > http://github.com/britter
>> > > >
>> > >
>> > >
>> > >
>> > > --
>> > > E-Mail: garydgregory@gmail.com | ggregory@apache.org
>> > > 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
>> >
>>
>>
>>
>> --
>> E-Mail: garydgregory@gmail.com | ggregory@apache.org
>> 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

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org


Mime
View raw message