2015-03-19 14:19 GMT+01:00 Werner Keil <[email protected]>: > What are the arguments here and why would it be from() rather than the > fairly common of()? > > well let say common is subjective since it is as common to not use it.
here semantically it is wrong since you dont get the builder of a configuration but you get a builder from a configuration. more root semantic would be unwrap or something like that - we can still use but istoo technical IMO. > from() generally suggests you convert one thing to another with the same > underlying semantics. > LocalDate.from() is an example > Obtains an instance of LocalDate from a temporal object. > Everywhere else that API usually calls it of(), too. > > The only exception in several places (not just JDK or JSRs) would be > parse() if the argument is strictly a string or textual format. > > If it's a complete no-op, create(), getInstance() or instance() would be > good alternatives, for those with arguments, see above. > > using the constructor just makes this topic closed IMO. The static method really brings nothing here. > Werner > > > > On Thu, Mar 19, 2015 at 2:08 PM, Romain Manni-Bucau <[email protected] > > > wrote: > > > why not from(xxx) directly? In any case your proposals sound better for > the > > frenchy I am than of(xxx) ;). > > > > > > Romain Manni-Bucau > > @rmannibucau <https://twitter.com/rmannibucau> | Blog > > <http://rmannibucau.wordpress.com> | Github < > > https://github.com/rmannibucau> | > > LinkedIn <https://www.linkedin.com/in/rmannibucau> | Tomitriber > > <http://www.tomitribe.com> > > > > 2015-03-19 13:40 GMT+01:00 Tresch, Anatole < > > [email protected] > > >: > > > > > I, saw, still an answer is required here ;) > > > > > > -----Original Message----- > > > From: Romain Manni-Bucau [mailto:[email protected]] > > > Sent: Montag, 16. März 2015 19:32 > > > To: [email protected] > > > Subject: Re: ConfigurationBuilder > > > > > > 2015-03-16 14:10 GMT+01:00 Werner Keil <[email protected]>: > > > > > > > Hi, > > > > > > > > The idea of a factory sounds good. I don't like the empty of() since > it > > > > goes against all the current APIs where of() is used, see Java SE 8 > > > > Date/Time, there is always something like of(argument) or > > ofMillis(int), > > > > etc. > > > > The only notable no-op factory methods are things like now(). > > > > > > > > So current() (also in CDI) seems understandable, ofCurrent() > > disturbing, > > > > but the idea OF having such static factory methods seems OK to me;-) > > > > > > > > Werner > > > > > > > > > > > > > > > > On Mon, Mar 16, 2015 at 1:55 PM, Anatole Tresch <[email protected]> > > > > wrote: > > > > > > > > > Hi all > > > > > > > > > > I have looked at the new builder module a bit more deeper and I > just > > > > write > > > > > down my suggestions here, so we can discuss them. Some of them also > > > might > > > > > be of general interest, regarding how APIs should be built in other > > > > modules > > > > > as well: > > > > > > > > > > > > > > > *Constructing a new ConfigurationBuilder:* > > > > > > > > > > Currently a user must type new ConfigurationBuilder(); to create a > > new > > > > > builder. How about using static factory methods for doing this (in > > JSR > > > > 354 > > > > > user feedback, e.g. from LJC was doing it that way would be > > preferred). > > > > As > > > > > an example, we could define the following: > > > > > > > > > > ConfigurationBuilder.of(); // creating a new empty > instance > > > > > ConfigurationBuilder.ofCurrent(); // create a new builder, using > > the > > > > > // current configuration as a > > > > > // starting point, allowing to > > > > easily > > > > > // adapt an existing > > > > configuration... > > > > > ConfigurationBuilder of(Configuration config); // create a new > > > builder, > > > > > // using the given > > > > > // configuration > > > > > > > > > > > +-0, using an inner class is as common as it AFAIK. That said I hate > "of" > > > for builder, doesnt mean anything for me. > > > > > > -> How about create(), createFromCurrentConfig(), > > > createFrom(Configuration) ? > > > > > > > > > > > *Adding new "items":* > > > > > > > > > > There are locations where methods are designed as follows: > > > > > > > > > > a) ConfigurationBuilder addItem(Item item, Item... items); > > > > > b) ConfigurationBuilder addItem(Item item); > > > > > > > > > > e.g. ConfigurationBuilder addPropertySource(...). > > > > > > > > > > -> Basically the API should be minimalistic, so > > > > > 1) I assume we can omit method b in that case. > > > > > 2) Collections (or Iterable) are not supported, so I would > think > > of > > > > > adding Collection support as > > > > > well. > > > > > 2) Since multiple instances could be passed, I would suggest to > > > name > > > > > add an 's' to its name. > > > > > 3) In "Effective Java" the variant with the single item in > front > > is > > > > > mentioned as an example, > > > > > where at least one instance must be passed. I personally > > think > > > > this > > > > > variant is > > > > > overengineered, since we can still either check the input, > > or > > > > > simply return (optionally > > > > > logging a warning). > > > > > > > > > > > > > > > Why not simply addItems(Item...items) - with or without the "s" I don't > > > care much > > > > > > -> I personally always find it confusing when its singular, but you can > > > multiple ones, whereas the other way round > > > a single one mathematically speaking can be seen as a subset of the > > > multiple ones, which the other way round is > > > not the case, so I prefer having the 's'... > > > > > > > > > > > Given that I would propose the signature to be looking as: > > > > > > > > > > a) ConfigurationBuilder addItems(Item... items); > > > > > b) ConfigurationBuilder addItems(Collection<Item> items); > > > > > > > > > > > > > > > -1 to have both - sounds opposed to what you just said, no? > > > -> 1:0 for you. Keep it simple... > > > > > > > > > > > > > *Boolean Property Signatures:* > > > > > > > > > > public boolean isPropertyConverterLoadingEnabled() {...} > > > > > public ConfigurationBuilder enableProvidedPropertyConverters() > {...} > > > > > public ConfigurationBuilder disableProvidedPropertyConverters() > {...} > > > > > > > > > > There are multiple boolean flags in the builder (the example ebove > > > shows > > > > > one of them). > > > > > I personally would expect the Java properties style to be applied > > here, > > > > so > > > > > it should be changed to: > > > > > > > > > > public boolean isPropertyConverterLoadingEnabled() {...} > > > > > public ConfigurationBuilder setProvidedPropertyConverters(boolean > > > > enabled) > > > > > {...} > > > > > > > > > > This reduces the API by 1 method. > > > > > > > > > > > > > > > For a builder on/off flag methods are common and more expressive. If we > > go > > > to setters I'd use setters eveywhere (see my other thread on this) > > > > > > -> Matter of taste IMO. I will not insist on this point... > > > > > > > > > > > > > *StringToURLMapper* > > > > > > > > > > I would suggest implementing this as a Lambda expression avoiding > the > > > > Java > > > > > class overhead. If compatibility was the focus with Java 7 it > should > > > not > > > > > extend java.util.function.Function, because this class is not > > available > > > > in > > > > > Java 7. > > > > > > > > > > > > > > > +-0 overhead = 0 since lambda implies the same class generation by the > > JVM > > > -> As above: I will not insist on this point... > > > > > > > > So far my feedback.Let me know what you think! > > > > > > > > > > Have a nice day! > > > > > Anatole > > > > > > > > > > > > > > > -- > > > > > *Anatole Tresch* > > > > > Java Engineer & Architect, JSR Spec Lead > > > > > Glärnischweg 10 > > > > > CH - 8620 Wetzikon > > > > > > > > > > *Switzerland, Europe Zurich, GMT+1* > > > > > *Twitter: @atsticks* > > > > > *Blogs: **http://javaremarkables.blogspot.ch/ > > > > > <http://javaremarkables.blogspot.ch/>* > > > > > > > > > > *Google: atsticksMobile +41-76 344 62 79* > > > > > > > > > > > > > > >
