You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@commons.apache.org by Benedikt Ritter <br...@apache.org> on 2013/04/07 20:14:23 UTC
Re: [CSV] Should the Builder API be optional?
Hey guys,
where are we standing with this? I see that Gary has added parse(Reader) to
the Builder as a short cut. We were talking about making the builder less
visible. How do you feel about renaming the newBuilder() methods to
newFormat()?
Benedikt
2013/3/26 sebb <se...@gmail.com>
> On 26 March 2013 16:53, Benedikt Ritter <br...@apache.org> wrote:
> > 2013/3/26 Gary Gregory <ga...@gmail.com>
> >
> >> On Tue, Mar 26, 2013 at 12:35 PM, sebb <se...@gmail.com> wrote:
> >>
> >> > On 26 March 2013 16:18, Benedikt Ritter <br...@apache.org> wrote:
> >> > > 2013/3/26 Gary Gregory <ga...@gmail.com>
> >> > >
> >> > >> On Tue, Mar 26, 2013 at 12:03 PM, Emmanuel Bourg <
> ebourg@apache.org>
> >> > >> wrote:
> >> > >>
> >> > >> > Le 26/03/2013 16:58, Gary Gregory a écrit :
> >> > >> >
> >> > >> > > Is it worth providing this shortcut:
> >> > >> > >
> >> > >> > > Iterable<CSVRecord> parse = CSVFormat.newBuilder().
> >> > >> > >
> >> >
> withCommentStart('#').withDelimiter('\t').withQuoteChar('"').parse(in);
> >> > >> > >
> >> > >> > > the builder would implement parse()
> >> > >> >
> >> > >> > I still think the builder is useless. My initial implementation
> >> looked
> >> > >> > like this:
> >> > >> >
> >> > >> > Iterable<CSVRecord> parse =
> >> > >> >
> >> > >> >
> >> > >>
> >> >
> >>
> CSVFormat.DEFAULT.withCommentStart('#').withDelimiter('\t').withQuoteChar('"').parse(in);
> >> > >> >
> >> > >> > So yes, in this spirit the builder should implement parse(). But
> I
> >> > would
> >> > >> > prefer no builder at all.
> >> > >> >
> >> > >>
> >> > >> By adding parse to the builder, we can make the builder less
> visible.
> >> I
> >> > >> think I'll add that.
> >> > >>
> >> > >
> >> > > This was my attention when I initially created the patch for the
> >> builder.
> >> > > the newBuilder() method was called newFormat() back then. We
> changed it
> >> > to
> >> > > not confuse users with what objects they are dealing with. With
> >> > > newBuilder() changed back to newFormat() again it would become:
> >> > >
> >> > > Iterable<CSVRecord> parse =
> >> > >
> >> >
> >>
> CSVFormat.newFormat().withCommentStart('#').withDelimiter('\t').withQuoteChar("'").parse(in);
> >> >
> >> > So effectively parse would do the build and then use it for the parse?
> >> >
> >>
> >> Yes (in SVN now) but I prefer:
> >>
> >>
> >>
> CSVFormat.DEFAULT.withCommentStart('#').withDelimiter('\t').withQuoteChar("'").parse(in);
> >>
> >> which does not create the need for calling newBuilder() and makes it
> >> obvious what it is I am building.
> >>
> >
> > So are we back to the inital design again or do you want to change
> DEFAULT
> > to hold a CSVBuilder?
> >
> >
> >>
> >> Gary
> >>
> >>
> >> > Is there a use-case for re-using the format without having to
> re-create
> >> it?
> >> > If so, how would that look?
> >>
> >
> > I'm not sure I understand your question. When parsing several files with
> > the same format one would reuse a CSVFormat instance and just create new
> > InputStreams for the files to parse.
>
> What I was getting at is what does the code look like to create the
> instance?
> Does this still use build() ?
>
> >
> >> >
> >> > > Benedikt
> >> > >
> >> > >
> >> > >>
> >> > >> Gary
> >> > >>
> >> > >> >
> >> > >> > Emmanuel Bourg
> >> > >> >
> >> > >> >
> >> > >> >
> >> > >>
> >> > >>
> >> > >> --
> >> > >> E-Mail: garydgregory@gmail.com | ggregory@apache.org
> >> > >> JUnit in Action, 2nd Ed: <http://goog_1249600977>
> http://bit.ly/ECvg0
> >> > >> Spring Batch in Action: <http://s.apache.org/HOq>
> http://bit.ly/bqpbCK
> >> > >> 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
> >> >
> >> >
> >>
> >>
> >> --
> >> E-Mail: garydgregory@gmail.com | ggregory@apache.org
> >> JUnit in Action, 2nd Ed: <http://goog_1249600977>http://bit.ly/ECvg0
> >> Spring Batch in Action: <http://s.apache.org/HOq>http://bit.ly/bqpbCK
> >> 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
>
>
--
http://people.apache.org/~britter/
http://www.systemoutprintln.de/
http://twitter.com/BenediktRitter
http://github.com/britter
Re: [CSV] Should the Builder API be optional?
Posted by Emmanuel Bourg <eb...@apache.org>.
Le 09/04/2013 08:48, Benedikt Ritter a écrit :
> You're right, the old impl was already immutable. The problem was, that it
> was not possible for a CSVFormat to validate itself at construction time.
> We had to call the package private validate() method. I think we have been
> through this several times. All arguments have been exchanged. If there is
> a way to let a CSVFormat validate itself by the time it is constructed with
> the old AP, I'm more than happy to drop the builder.
The need for the validation in exaggerated in my opinion. The format was
validated at parse time before and that was fine. If you feel it's
important to validate earlier I suggest adding a public validate()
method and let the user call it if needed.
Emmanuel Bourg
Re: [CSV] Should the Builder API be optional?
Posted by Benedikt Ritter <br...@apache.org>.
2013/4/10 Oliver Heger <ol...@oliver-heger.de>
> Am 09.04.2013 08:48, schrieb Benedikt Ritter:
>
> 2013/4/8 Emmanuel Bourg <eb...@apache.org>
>>
>> Le 08/04/2013 22:39, Gary Gregory a écrit :
>>>
>>> But that's the price for immutability for some of these objects.
>>>>
>>>
>>> Not sure, we already achieved immutability last year without paying this
>>> price:
>>>
>>>
>>> http://svn.apache.org/repos/**asf/commons/proper/csv/trunk/**
>>> src/main/java/org/apache/**commons/csv/CSVFormat.java?p=**1305548<http://svn.apache.org/repos/asf/commons/proper/csv/trunk/src/main/java/org/apache/commons/csv/CSVFormat.java?p=1305548>
>>>
>>> This design was sacrified for the sake of implementing a "by the book"
>>> builder pattern that brings no real benefit in term of usability. It's
>>> just a useless layer of complexity.
>>>
>>>
>> You're right, the old impl was already immutable. The problem was, that it
>> was not possible for a CSVFormat to validate itself at construction time.
>> We had to call the package private validate() method. I think we have been
>> through this several times. All arguments have been exchanged. If there is
>> a way to let a CSVFormat validate itself by the time it is constructed
>> with
>> the old AP, I'm more than happy to drop the builder.
>>
>> This is not about implementing "by the book" builder patterns.
>>
>
> Not sure whether this is worth the effort, but maybe as an academic
> exercise in defining DSLs with Java:
>
> Would an approach like the following one be acceptable from an API user's
> point of view:
>
> CSVFormat newFormat = CSVFormat.DEFAULT.derive(
> withDelimiter('!'), withNullString("NULL"));
>
I like this. With a slight twist it would become even shorter:
CSVFormat newFormat = CSVFormat.DEFAULT.with(
delimiter('!'), nullString("NULL"));
This may even help with CSV-78 [1] because we could do something like:
CSVFormat newFormat = CSVFormat.DEFAULT.with(noEscape());
>
> Here the derive() method expects a number of FormatManipulator objects as
> varargs argument. The withXXX() methods are static methods either in
> CSVFormat or a separate helper class creating concrete FormatManipulator
> implementations.
>
> derive() creates a mutable format definition object based on its current
> state. It iterates over its arguments passing each the format definition
> object. Concrete manipulator implementations update it accordingly. At the
> end, the resulting format definition is validated, and a new immutable
> CSVFormat is created from it.
>
> This is obviously some implementation effort. It requires less code for
> the user of the API. The approach is extensible, new properties can be
> added by providing new manipulator implementations and static convenience
> methods for creating them.
>
Yep this does seem to be a little more complex. But if we can satisfy all
people involved here and create a good API from a user POV it is worth the
effort IMHO.
Thank you very much for bringing in a fresh new idea in this disussion!
Benedikt
[1] https://issues.apache.org/jira/browse/CSV-78
>
> Oliver
>
>
>> Benedikt
>>
>>
>>
>>>
>>> Emmanuel Bourg
>>>
>>>
>>>
>>>
>>
>>
>
> ------------------------------**------------------------------**---------
> To unsubscribe, e-mail: dev-unsubscribe@commons.**apache.org<de...@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
Re: [CSV] Should the Builder API be optional?
Posted by Oliver Heger <ol...@oliver-heger.de>.
Am 09.04.2013 08:48, schrieb Benedikt Ritter:
> 2013/4/8 Emmanuel Bourg <eb...@apache.org>
>
>> Le 08/04/2013 22:39, Gary Gregory a écrit :
>>
>>> But that's the price for immutability for some of these objects.
>>
>> Not sure, we already achieved immutability last year without paying this
>> price:
>>
>>
>> http://svn.apache.org/repos/asf/commons/proper/csv/trunk/src/main/java/org/apache/commons/csv/CSVFormat.java?p=1305548
>>
>> This design was sacrified for the sake of implementing a "by the book"
>> builder pattern that brings no real benefit in term of usability. It's
>> just a useless layer of complexity.
>>
>
> You're right, the old impl was already immutable. The problem was, that it
> was not possible for a CSVFormat to validate itself at construction time.
> We had to call the package private validate() method. I think we have been
> through this several times. All arguments have been exchanged. If there is
> a way to let a CSVFormat validate itself by the time it is constructed with
> the old AP, I'm more than happy to drop the builder.
>
> This is not about implementing "by the book" builder patterns.
Not sure whether this is worth the effort, but maybe as an academic
exercise in defining DSLs with Java:
Would an approach like the following one be acceptable from an API
user's point of view:
CSVFormat newFormat = CSVFormat.DEFAULT.derive(
withDelimiter('!'), withNullString("NULL"));
Here the derive() method expects a number of FormatManipulator objects
as varargs argument. The withXXX() methods are static methods either in
CSVFormat or a separate helper class creating concrete FormatManipulator
implementations.
derive() creates a mutable format definition object based on its current
state. It iterates over its arguments passing each the format definition
object. Concrete manipulator implementations update it accordingly. At
the end, the resulting format definition is validated, and a new
immutable CSVFormat is created from it.
This is obviously some implementation effort. It requires less code for
the user of the API. The approach is extensible, new properties can be
added by providing new manipulator implementations and static
convenience methods for creating them.
Oliver
>
> Benedikt
>
>
>>
>>
>> Emmanuel Bourg
>>
>>
>>
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [CSV] Should the Builder API be optional?
Posted by Benedikt Ritter <br...@apache.org>.
2013/4/8 Emmanuel Bourg <eb...@apache.org>
> Le 08/04/2013 22:39, Gary Gregory a écrit :
>
> > But that's the price for immutability for some of these objects.
>
> Not sure, we already achieved immutability last year without paying this
> price:
>
>
> http://svn.apache.org/repos/asf/commons/proper/csv/trunk/src/main/java/org/apache/commons/csv/CSVFormat.java?p=1305548
>
> This design was sacrified for the sake of implementing a "by the book"
> builder pattern that brings no real benefit in term of usability. It's
> just a useless layer of complexity.
>
You're right, the old impl was already immutable. The problem was, that it
was not possible for a CSVFormat to validate itself at construction time.
We had to call the package private validate() method. I think we have been
through this several times. All arguments have been exchanged. If there is
a way to let a CSVFormat validate itself by the time it is constructed with
the old AP, I'm more than happy to drop the builder.
This is not about implementing "by the book" builder patterns.
Benedikt
>
>
> Emmanuel Bourg
>
>
>
--
http://people.apache.org/~britter/
http://www.systemoutprintln.de/
http://twitter.com/BenediktRitter
http://github.com/britter
Re: [CSV] Should the Builder API be optional?
Posted by Emmanuel Bourg <eb...@apache.org>.
Le 09/04/2013 07:22, Gary Gregory a écrit :
> WRT org.apache.commons.csv.CSVFormat.CSVFormat(char, Character, Quote,
> Character, Character, boolean, boolean, String, String, String[])
>
> There does not seem to be a good reason why this is not public. The only
> argument I've heard is that some people do not like to use long ctors. But
> so what? If we make it public, users have the choice to the the whole
> fluent builder API or not.
>
> Thoughts?
The good reason is to prevent the proliferation of insanely long and
unusable constructors.
Today you have a constructor with 10 parameters, tomorrow as the API
evolves new constructors will appear with 11, 12, 13 parameters. This is
just messy.
Emmanuel Bourg
Re: [CSV] Should the Builder API be optional?
Posted by Adrian Crum <ad...@sandglass-software.com>.
True. Users are free to create their own facade to make object
construction easier.
-Adrian
On 4/9/2013 6:22 AM, Gary Gregory wrote:
> WRT org.apache.commons.csv.CSVFormat.CSVFormat(char, Character, Quote,
> Character, Character, boolean, boolean, String, String, String[])
>
> There does not seem to be a good reason why this is not public. The only
> argument I've heard is that some people do not like to use long ctors. But
> so what? If we make it public, users have the choice to the the whole
> fluent builder API or not.
>
> Thoughts?
>
> Gary
>
>
> On Mon, Apr 8, 2013 at 7:15 PM, Gary Gregory <ga...@gmail.com> wrote:
>
>> I would be ok with making the parser and format ctors public. What
>> else? I agree that we should not force force folks into an API pattern
>> but here it's not a big API at least.
>>
>> Gary
>>
>> On Apr 8, 2013, at 17:02, Emmanuel Bourg <eb...@apache.org> wrote:
>>
>>> Le 08/04/2013 22:39, Gary Gregory a écrit :
>>>
>>>> But that's the price for immutability for some of these objects.
>>> Not sure, we already achieved immutability last year without paying this
>>> price:
>>>
>>>
>> http://svn.apache.org/repos/asf/commons/proper/csv/trunk/src/main/java/org/apache/commons/csv/CSVFormat.java?p=1305548
>>> This design was sacrified for the sake of implementing a "by the book"
>>> builder pattern that brings no real benefit in term of usability. It's
>>> just a useless layer of complexity.
>>>
>>>
>>> Emmanuel Bourg
>>>
>>>
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [CSV] Should the Builder API be optional?
Posted by Benedikt Ritter <br...@apache.org>.
2013/4/9 Gary Gregory <ga...@gmail.com>
> WRT org.apache.commons.csv.CSVFormat.CSVFormat(char, Character, Quote,
> Character, Character, boolean, boolean, String, String, String[])
>
> There does not seem to be a good reason why this is not public. The only
> argument I've heard is that some people do not like to use long ctors. But
> so what? If we make it public, users have the choice to the the whole
> fluent builder API or not.
>
> Thoughts?
>
I think this doesn't solve the problem that the builder is more noisy than
the old API. But right now I have no clue how to make it even more
simple/fluent/whatever.
Benedikt
>
> Gary
>
>
> On Mon, Apr 8, 2013 at 7:15 PM, Gary Gregory <ga...@gmail.com>
> wrote:
>
> > I would be ok with making the parser and format ctors public. What
> > else? I agree that we should not force force folks into an API pattern
> > but here it's not a big API at least.
> >
> > Gary
> >
> > On Apr 8, 2013, at 17:02, Emmanuel Bourg <eb...@apache.org> wrote:
> >
> > > Le 08/04/2013 22:39, Gary Gregory a écrit :
> > >
> > >> But that's the price for immutability for some of these objects.
> > >
> > > Not sure, we already achieved immutability last year without paying
> this
> > > price:
> > >
> > >
> >
> http://svn.apache.org/repos/asf/commons/proper/csv/trunk/src/main/java/org/apache/commons/csv/CSVFormat.java?p=1305548
> > >
> > > This design was sacrified for the sake of implementing a "by the book"
> > > builder pattern that brings no real benefit in term of usability. It's
> > > just a useless layer of complexity.
> > >
> > >
> > > Emmanuel Bourg
> > >
> > >
> >
>
>
>
> --
> E-Mail: garydgregory@gmail.com | ggregory@apache.org
> JUnit in Action, 2nd Ed: <http://goog_1249600977>http://bit.ly/ECvg0
> Spring Batch in Action: <http://s.apache.org/HOq>http://bit.ly/bqpbCK
> 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
Re: [CSV] Should the Builder API be optional?
Posted by Gary Gregory <ga...@gmail.com>.
WRT org.apache.commons.csv.CSVFormat.CSVFormat(char, Character, Quote,
Character, Character, boolean, boolean, String, String, String[])
There does not seem to be a good reason why this is not public. The only
argument I've heard is that some people do not like to use long ctors. But
so what? If we make it public, users have the choice to the the whole
fluent builder API or not.
Thoughts?
Gary
On Mon, Apr 8, 2013 at 7:15 PM, Gary Gregory <ga...@gmail.com> wrote:
> I would be ok with making the parser and format ctors public. What
> else? I agree that we should not force force folks into an API pattern
> but here it's not a big API at least.
>
> Gary
>
> On Apr 8, 2013, at 17:02, Emmanuel Bourg <eb...@apache.org> wrote:
>
> > Le 08/04/2013 22:39, Gary Gregory a écrit :
> >
> >> But that's the price for immutability for some of these objects.
> >
> > Not sure, we already achieved immutability last year without paying this
> > price:
> >
> >
> http://svn.apache.org/repos/asf/commons/proper/csv/trunk/src/main/java/org/apache/commons/csv/CSVFormat.java?p=1305548
> >
> > This design was sacrified for the sake of implementing a "by the book"
> > builder pattern that brings no real benefit in term of usability. It's
> > just a useless layer of complexity.
> >
> >
> > Emmanuel Bourg
> >
> >
>
--
E-Mail: garydgregory@gmail.com | ggregory@apache.org
JUnit in Action, 2nd Ed: <http://goog_1249600977>http://bit.ly/ECvg0
Spring Batch in Action: <http://s.apache.org/HOq>http://bit.ly/bqpbCK
Blog: http://garygregory.wordpress.com
Home: http://garygregory.com/
Tweet! http://twitter.com/GaryGregory
Re: [CSV] Should the Builder API be optional?
Posted by Gary Gregory <ga...@gmail.com>.
I would be ok with making the parser and format ctors public. What
else? I agree that we should not force force folks into an API pattern
but here it's not a big API at least.
Gary
On Apr 8, 2013, at 17:02, Emmanuel Bourg <eb...@apache.org> wrote:
> Le 08/04/2013 22:39, Gary Gregory a écrit :
>
>> But that's the price for immutability for some of these objects.
>
> Not sure, we already achieved immutability last year without paying this
> price:
>
> http://svn.apache.org/repos/asf/commons/proper/csv/trunk/src/main/java/org/apache/commons/csv/CSVFormat.java?p=1305548
>
> This design was sacrified for the sake of implementing a "by the book"
> builder pattern that brings no real benefit in term of usability. It's
> just a useless layer of complexity.
>
>
> Emmanuel Bourg
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [CSV] Should the Builder API be optional?
Posted by Emmanuel Bourg <eb...@apache.org>.
Le 08/04/2013 22:39, Gary Gregory a écrit :
> But that's the price for immutability for some of these objects.
Not sure, we already achieved immutability last year without paying this
price:
http://svn.apache.org/repos/asf/commons/proper/csv/trunk/src/main/java/org/apache/commons/csv/CSVFormat.java?p=1305548
This design was sacrified for the sake of implementing a "by the book"
builder pattern that brings no real benefit in term of usability. It's
just a useless layer of complexity.
Emmanuel Bourg
Re: [CSV] Should the Builder API be optional?
Posted by Gary Gregory <ga...@gmail.com>.
On Mon, Apr 8, 2013 at 4:35 PM, Emmanuel Bourg <eb...@apache.org> wrote:
> Le 07/04/2013 20:14, Benedikt Ritter a écrit :
>
> > where are we standing with this? I see that Gary has added parse(Reader)
> to
> > the Builder as a short cut. We were talking about making the builder less
> > visible. How do you feel about renaming the newBuilder() methods to
> > newFormat()?
>
> Sure why not, but that's not enough in my opinion.
>
> I used to be able to write this:
>
> fmt = CSVFormat.DEFAULT.withDelimiter('!');
>
> and now I have to write this:
>
> fmt = CSVFormat.newBuilder().withDelimiter('!').build()
>
Right, I just wrote:
final CSVPrinter printer = new CSVPrinter(sw,
CSVFormat.DEFAULT.toBuilder().withNullToString("NULL").build());
Yikes! But that's the price for immutability for some of these objects.
Adding shortcuts would bloat the code:
CSVFormat.DEFAULT.buildWithNullToString("NULL");
I'd need a buildWith method for each format option. Do-able but not great.
Gary
>
> That's much longer :(
>
>
> Emmanuel Bourg
>
>
>
--
E-Mail: garydgregory@gmail.com | ggregory@apache.org
JUnit in Action, 2nd Ed: <http://goog_1249600977>http://bit.ly/ECvg0
Spring Batch in Action: <http://s.apache.org/HOq>http://bit.ly/bqpbCK
Blog: http://garygregory.wordpress.com
Home: http://garygregory.com/
Tweet! http://twitter.com/GaryGregory
Re: [CSV] Should the Builder API be optional?
Posted by Emmanuel Bourg <eb...@apache.org>.
Le 07/04/2013 20:14, Benedikt Ritter a écrit :
> where are we standing with this? I see that Gary has added parse(Reader) to
> the Builder as a short cut. We were talking about making the builder less
> visible. How do you feel about renaming the newBuilder() methods to
> newFormat()?
Sure why not, but that's not enough in my opinion.
I used to be able to write this:
fmt = CSVFormat.DEFAULT.withDelimiter('!');
and now I have to write this:
fmt = CSVFormat.newBuilder().withDelimiter('!').build()
That's much longer :(
Emmanuel Bourg