commons-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Matt Benson <gudnabr...@gmail.com>
Subject Re: [CSV] Creating CSVParser instances
Date Wed, 14 Aug 2013 13:12:47 GMT
To stir the pot, one might even make a case for using Reader only ;-) .

Matt
On Aug 14, 2013 8:03 AM, "Gary Gregory" <garydgregory@gmail.com> wrote:

> On Wed, Aug 14, 2013 at 2:44 AM, 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?
> >
>
> Go for it.
>
> Gary
>
>
> > > >
> > >
> > > 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.
> >
> > Would it be okay for you if we remove those methods? 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.
> >
> > 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
> >
>
>
>
> --
> 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
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message