Thursday, May 11, 2017, 6:49:09 AM, Woonsan Ko wrote: > Hi Daniel, > > On Fri, May 5, 2017 at 3:18 PM, Daniel Dekany <ddek...@apache.org> wrote: >> This is now partially implemented (I have just committed it). There >> are many rough edges yet, but Configuration is now immutable, and it >> basically works. See: >> >> >> https://github.com/apache/incubator-freemarker/blob/3/src/test/java/org/apache/freemarker/manualtest/GettingStartedExample.java >> >> Would be good if more people looks at this thing. As you will see if >> you do, there's a hierarchy of configuration related interfaces, which >> are implemented by both XxxConfiguration and XxxConfiguration.Builder. >> And so on... please find mistakes in this stuff, especially >> architectural ones. > > It looks really good! Great work!
Speaking of the XxxConfiguration interfaces... If we want to support specifying ObjectWrapper-s and such composite values with a builder instead of with the final object (see earlier mails, partially still quoted at end), then the builder classes can't implement the XxxConfiguration interface, which would be quite a loss. They can't, because for example, now we have ProcessingConfiguration.getObjectWrapper(), but according the plan the builder will only have ProcessingConfiguration.getObjectWrapperBuilder(), and setObjectWrapper(ObjectWrapper) is just a convenience method that creates a dummy builder and delegates to setObjectWrapperBuilder(ObjectWrapperBuilder). Given that I expect this "layered configuration" thing to be a very rare demand, and having it would complicate the public API for everyone (I couldn't see this problem until I have implemented what we have now), I guess it doesn't worth doing it after all. If someone will need this feature, unfortunately, they will have to add their own builder facade that postpones the creation of the ObjectWrapper (and such) from the ObjectWrapperBuilder until it's know that ObjectWrapperBuilder won't be modified anymore. (BTW, FreemarkerServlet got away without needing this feature, so we are good there.) >> There are some missing and somewhat undecided things, most notably: >> >> - If a setting has a Map or List value, when someone calls >> builder.setFoo(someMap), should we copy someMap, or let people >> modify it via aliasing? Well, at the very latest when >> builder.build() is called, we must copy it, as the result must be >> immutable. Also, aliasing has this problem that when the user >> utilizes it, it's potentially confusing for someone else reading the >> code later, as instinctively you only consider the content of the >> Map (or List, etc.) where builder.setFoo(someMap) was called. Also >> if you utilize aliasing like builder.getFoo().put(some, more), then >> you possibly unwillingly assume that the Map passed into setFoo was >> mutable (not a Guava ImmutableMao.of(...), which is popular). So I'm >> bending towards copying the map at the setter, and return an >> immutable view with builder.getFoo(), but add a dedicated >> `builder.putFoo(some, more)` (as it was in FM2 as well). This allows >> us to do some performance tricks, like cache invalidation in >> Environment. > > +1. Fully agree to all the pointers you made. I will do this then, unless someone else has something to add. >> - Should we allow calling builder.build() for multiple times? The >> trap one can run into with that is that then the two (or more) >> resulting objects will share the same stateful setting values, like >> the cacheStorage. That can lead to malfunction, and especially as >> cacheStorage has a default value, it's a quite sneaky thing >> (because, the user doesn't even realize that such setting exists, as >> he did not set it explicitly). > > I think a builder instance may return the same instance. I've read > about the builder pattern again: > https://en.wikipedia.org/wiki/Builder_pattern. There's no rule or > reason to have to return a new instance on every #build() call. Even > the Java example, CarBuilderImpl, in the page returns the same > instance, too. It seems a lot safer to do in our context as well. So, > I would return the same instance from the same builder on #build() > calls. But that's impossible if the builder creates immutable objects (which is the main reason of using builders in many Java frameworks, including FM3): Object obj1 = builder.build(); b1.setFoo(123); Object obj2 = builder.build(); assertSame(obj1, obj2) // Impossible, as we create immutable objects > Kind regards, > > Woonsan > >> >> >> Sunday, February 19, 2017, 7:41:54 PM, Woonsan Ko wrote: >> >>> On Sun, Feb 19, 2017 at 10:04 AM, Daniel Dekany <ddek...@freemail.hu> wrote: >>>> Sunday, February 19, 2017, 11:35:44 AM, Denis Bredelet wrote: >>>> >>>>> My message was not clear, I mean builders are not required for >>>>> standard configuration settings if they are simple types. >>>> >>>> Of course not. We are only talking about setting values like >>>> ObjectWrapper, TemplateLoader, TemplateResolver (when it's added), >>>> etc. For them, I think we will have just bite the bullet and add both >>>> `void setFoo(Foo)`, and `void setFooBuilder(Builder<Foo>)` (and their >>>> fluent API equivalent), and `Builder<Foo> getFooBuilder()`, but no Foo >>>> getFoo(). >>> >>> +1 >>> >>>> And if you call setFoo(myFoo), then getFooBuilder() will >>>> return something like a `new o.a.f.core.ResultWrapperBuilder(myFoo)`, >>>> where ResultWrapperBuilder is just a wrapper myFoo, so that it >>>> satisfies the Builder<Foo> interface. >>> >>> +1 >>> >>>> I think I will also ban using >>>> setFoo(Foo) if Foo is known to have a Builder, just to enforce the >>>> intended usage pattern. >>> >>> +1 >>> >>>> Unless somebody comes up with a better idea, I >>>> will try this approach when I get there, and we will see how it works >>>> out in reality. >>> >>> +1 >>> >>> Woonsan >>> >>>> >>>>>> Le 19 févr. 2017 à 10:24, Denis Bredelet <brede...@mac.com> a écrit : >>>>>> >>>>>> Hi Daniel, >>>>>> >>>>>>>>> But then we lose the immutability of the built object, which was the >>>>>>>>> point of using builders on the first place. >>>>>>>> >>>>>>>> Ah, you're totally right! If builder is able to update an immutable >>>>>>>> object, then the object turns out to be mutable indirectly through the >>>>>>>> builder. I missed that part. :-) >>>>>>>> Hmm.. it seems difficult to make it immutable considering those >>>>>>>> expensive nested objects. >>>>>>>> Would it be possible to introduce lifecycle methods such as >>>>>>>> #initialize() in Configuration to prohibit (IllegalStateException) >>>>>>>> using it and its nested objects until initialized? >>>>>>> >>>>>>> Sure, but that was plan B in case we don't go for builders at all (see >>>>>>> in the thread starter mail). I mean, if we have that, what's the >>>>>>> benefit of the builder classes? When I go for builders, I would hope >>>>>>> fore real final fields in the built objects, and that also means that >>>>>>> I don't have to worry about the ordering of the field assignments, >>>>>>> because I got all everything at once, so I can sort the assignments. >>>>>> >>>>>> Final fields for built objects is not a requirement if all the standard >>>>>> configuration settings are simple, e.g. String or Integer etc. >>>>>> >>>>>> Then possible mutations do not affect the standard configuration when >>>>>> using a builder. >>>>>> >>>>>> — Denis. >>>>>> >>>>>>>> If so, perhaps we can wait for everything configured in each stage >>>>>>>> and finally initialize those expensive objects only once by the >>>>>>>> core. Also, would it be possible to wait until the final >>>>>>>> initialization phase, only capturing settings, without having to >>>>>>>> create a nested object such as ObjecrWrapper in earlier stage? >>>>>>> >>>>>>> Where do I capture the settings of the ObjectWrapper (the nested >>>>>>> settings)? Anyway, that's why I thought that perhaps, if we have >>>>>>> cfg.setFoo(T) where Foo is not just some primitive or such, then we >>>>>>> also allow cfg.setFooBuilder(Builder<? extends T>)... and then, you >>>>>>> can, using your words, capture the nested settings too. >>>>>> >>>>> >>>>> >>>> >>>> -- >>>> Thanks, >>>> Daniel Dekany >>>> >>> >> >> -- >> Thanks, >> Daniel Dekany >> > -- Thanks, Daniel Dekany
