On Mon, Jul 21, 2014 at 3:26 PM, Benedikt Ritter <[email protected]> wrote:
> 2014-07-21 21:15 GMT+02:00 Gary Gregory <[email protected]>: > > > On Mon, Jul 21, 2014 at 3:05 PM, Benedikt Ritter <[email protected]> > > wrote: > > > > > 2014-07-21 20:55 GMT+02:00 Gary Gregory <[email protected]>: > > > > > > > On Mon, Jul 21, 2014 at 2:45 PM, Benedikt Ritter <[email protected] > > > > > > wrote: > > > > > > > > > Hey Gary, > > > > > > > > > > I like what you did with the isEscaping, isHandlingNull etc. The > API > > > > looks > > > > > much cleaner to me now. > > > > > > > > > > What I don't understand is, why you added the type postfix to some > of > > > the > > > > > getters. If we have getCommentStartCharacter, why don't we have > > > > > getRecordSeparatorString ? > > > > > > > > > > > > > No good reason. > > > > > > > > > > :) > > > > > > How about removing the postfixes again? > > > > > > If the requirement for additional getters returning different typs come > > up, > > > we can still add getters with postfix: > > > > > > CSV v1.0: > > > getCommentStart() > > > > > > CSV v1.1 > > > getCommentStart() > > > getCommentStartString() / getCommentStartAsString() > > > > > > WDYT? > > > > > > > > I'm not crazy about it because the APIs read oddly otherwise: > > > > For example getEscapeCharacter() is clear to me and means -- to me -- > > returns the escape character. The only complaint I can imagine is > > verbosity. But taking it out is odd "get escape" means get escape what? > Is > > escape enabled, disabled? Some attribute of escaping? So > getEscapeCharacter > > is good as is IMO. > > > > > So you had a good reason ;-) > > one could say, that the information what kind (of escape) is being returned > is encoded in the method signature, because it is returning a Character. > But I'm fine with leaving the type information in the method name, where it > is not clear. That said getDelimiter() is pretty clear to me, as well as > getRecordSeparator. > That's a good point but less obvious when you read code. You need to know the return type or the type of the object your putting the value. It's 50/50 I guess. > > > > getQuoteCharacter is good because we have more than one item to configure > > for quote: getQuoteMode and getQuoteCharacter. I can see that because of > > this specific property, getQuote is understandable, but I prefer > > consistency of getQuote<Attribute>. > > > > Agreed. > > > > > > getCommentStartCharacter() is good because it is clean getCommentStart > > means what? Start could be a misinterpreted as verb, as in "comment > > starting is enabled" or "commenting is enabled" which is not what this > > property is about. > > > > Emmanuel already commented on that particular method. We renamed it to > getCommentMarker(), but it looks like that has been changed again. > > Let's change that back to getCommentMarker() again and leave the rest as it > is. > Ah, yes, that got messed up it seems. Please go ahead and make adjustments. Gary > > > > > > Gary > > > > > > > > > > > Benedikt > > > > > > > > > > > > > > Gary > > > > > > > > > > > > > > > > > > We're very close... > > > > > > > > > > Benedikt > > > > > > > > > > > > > > > 2014-07-21 19:18 GMT+02:00 Gary Gregory <[email protected]>: > > > > > > > > > > > On Mon, Jul 21, 2014 at 12:35 PM, Benedikt Ritter < > > > [email protected]> > > > > > > wrote: > > > > > > > > > > > > > Please take another look at CSV in rev. 1612344. I have: > > > > > > > > > > > > > > - renamed the ignoreEmptyHeaders property to > > > allowMissingColumnNames > > > > > > > - prefixed everything that is a pure getter with "get" > > > > > > > - renamed Quote/QuotePolicy consistently to QuoteMode > > > > > > > > > > > > > > > > > > > Good changes! > > > > > > > > > > > > > > > > > > > Is this the final API we can agree upon? > > > > > > > > > > > > > > > > > > > Not quite, these names are still bad IMO: > > > > > > > > > > > > org.apache.commons.csv.CSVFormat.isHandlingNull() > > > > > > > > > > > > Terrible name, isNullStringSet() is simpler and expresses what > the > > > code > > > > > > does. FWIW, It is also the same kind of name JAXB generates for > > this > > > > code > > > > > > pattern. > > > > > > > > > > > > org.apache.commons.csv.CSVFormat.isQuoting() > > > > > > > > > > > > isFoo should return a foo ivar. This is not the case here. > > > > > > > > > > > > org.apache.commons.csv.CSVFormat.isCommentingEnabled() return a > > > > computed, > > > > > > so the name is good. > > > > > > > > > > > > Same for org.apache.commons.csv.CSVFormat.isEscaping() but why > does > > > one > > > > > > have the "Enabled" postfix and not the other? The names should be > > > > > > consistent, I say remove "Enabled". > > > > > > > > > > > > So to summarize, looking at all of: > > > > > > > > > > > > isCommentingEnabled() > > > > > > isEscaping() > > > > > > isHandlingNull() > > > > > > isQuoting() > > > > > > > > > > > > they all do the same thing: test an ivar for null. > > > > > > > > > > > > So the simple thing to do is to make sure we have a good ivar > name > > > and > > > > > use > > > > > > the JAXB inspired pattern: ivar foo has a test method called > > > isFooSet() > > > > > > > > > > > > commentStart : Character > > > > > > escape : Character > > > > > > quoteChar : Character > > > > > > > > > > > > First, why is one name postFixed with Char and not the others? > That > > > > > should > > > > > > be consistent. And why not use the full name "Character" in the > > > > postfix? > > > > > > Let's do that and match the method names. > > > > > > > > > > > > This also gives us room for String versions later, > > > > > getCommentStartString() > > > > > > for example, without breaking BC. > > > > > > > > > > > > For the with* methods, having the Character postfix does not make > > > > sense, > > > > > > since the type of the argument is in the signature. So > > > > > > withQuoteChar(char|Character) should be come > > > withQuote(char|Character). > > > > > > > > > > > > Now, that's better :-) > > > > > > > > > > > > My only question now is should "char delimiter" be "char > > > > delimiterChar"? > > > > > > > > > > > > See revision 1612352. > > > > > > > > > > > > Gary > > > > > > > > > > > > > > > > > > > > > > > > > > br, > > > > > > > Benedikt > > > > > > > > > > > > > > > > > > > > > 2014-07-20 14:04 GMT+02:00 Gary Gregory < > [email protected] > > >: > > > > > > > > > > > > > > > I like using all "get" methods and no "is" methods. It is > > simpler > > > > and > > > > > > > > makes the getters easier to access as a group with code > > > completion > > > > > IMO. > > > > > > > The > > > > > > > > with methods do not behave like Java bean method so I do not > > > thing > > > > we > > > > > > > need > > > > > > > > to worry about that. Unless we want to register > immutability... > > > > > > > > > > > > > > > > Gary > > > > > > > > > > > > > > > > <div>-------- Original message --------</div><div>From: > > Benedikt > > > > > > Ritter < > > > > > > > > [email protected]> </div><div>Date:07/20/2014 04:02 > > > (GMT-05:00) > > > > > > > > </div><div>To: Commons Developers List < > [email protected] > > > > > > > > > > > </div><div>Subject: Re: [CSV] Naming pattern of getters and > > > setters > > > > > in > > > > > > > > CSVFormat (was: Re: [VOTE] Release Commons CSV 1.0 based on > > RC1) > > > > > > > </div><div> > > > > > > > > </div>using "get" for methods that return booleans is very > > > uncommon > > > > > > > imho... > > > > > > > > > > > > > > > > how about leaving all the gramme stuff out and use: > > > > > > > > > > > > > > > > void withSkipEmptyHeaders(boolean) > > > > > > > > boolean isSkipEmptyHeaders > > > > > > > > > > > > > > > > that would > > > > > > > > - restore symmetry between getter and setter > > > > > > > > - almost follow JavaBean conventions (except for the "with") > > > > > > > > > > > > > > > > br, > > > > > > > > Benedikt > > > > > > > > > > > > > > > > > > > > > > > > 2014-07-20 8:00 GMT+02:00 Dipanjan Laha < > [email protected] > > >: > > > > > > > > > > > > > > > > > Although i am not familiar with CSV's codebase, imho "get" > is > > > > more > > > > > > > > straight > > > > > > > > > forward, so +1 to Gary's suggestion. > > > > > > > > > > > > > > > > > > On Saturday, 19 July 2014, Gary Gregory < > > > [email protected]> > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > On Sat, Jul 19, 2014 at 12:14 PM, Emmanuel Bourg < > > > > > > [email protected] > > > > > > > > > > <javascript:;>> wrote: > > > > > > > > > > > > > > > > > > > > > Le 19/07/2014 13:48, Gary Gregory a écrit : > > > > > > > > > > > > > > > > > > > > > > > Can we go back to use "get"? > > > > > > > > > > > > > > > > > > > > > > We are running in circles Gary, Benedikt and I, if > others > > > > could > > > > > > > weigh > > > > > > > > > in > > > > > > > > > > > that would help. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Circles, back and forth, to and fro, call it what you > will. > > > IMO > > > > > > this > > > > > > > is > > > > > > > > > the > > > > > > > > > > nature of the kind of development we do. Decentralized, > no > > > > water > > > > > > > > cooler, > > > > > > > > > no > > > > > > > > > > white board, all emails, leads to this development style, > > > which > > > > > is > > > > > > > what > > > > > > > > > we > > > > > > > > > > have to live with. > > > > > > > > > > > > > > > > > > > > In this case, it seems we had to try the code several > ways > > > and > > > > > see > > > > > > it > > > > > > > > > > before we can decide. In an office, we might have decided > > in > > > > pair > > > > > > > > > > programming in 5 minutes, this is not what we have. That > or > > > > > > architect > > > > > > > > > would > > > > > > > > > > have created some coding edict that imposes coding style. > > > > > > > > > > > > > > > > > > > > So this circling is all OK by me ;-) > > > > > > > > > > > > > > > > > > > > Gary > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Emmanuel Bourg > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > --------------------------------------------------------------------- > > > > > > > > > > > To unsubscribe, e-mail: > > [email protected] > > > > > > > > > > <javascript:;> > > > > > > > > > > > For additional commands, e-mail: > > > [email protected] > > > > > > > > > > <javascript:;> > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > -- > > > > > > > > > > E-Mail: [email protected] <javascript:;> | > > > > > > [email protected] > > > > > > > > > > <javascript:;> > > > > > > > > > > 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 > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > -- > > > > > > > http://people.apache.org/~britter/ > > > > > > > http://www.systemoutprintln.de/ > > > > > > > http://twitter.com/BenediktRitter > > > > > > > http://github.com/britter > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > -- > > > > > > E-Mail: [email protected] | [email protected] > > > > > > 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: [email protected] | [email protected] > > > > 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: [email protected] | [email protected] > > 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: [email protected] | [email protected] 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
