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! > > 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. > > - 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. 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 >
