On Sun, May 21, 2017 at 5:44 AM, Daniel Dekany <[email protected]> wrote: > Friday, May 12, 2017, 10:49:33 PM, Daniel Dekany wrote: > >> Thursday, May 11, 2017, 6:49:09 AM, Woonsan Ko wrote: >> >>> Hi Daniel, >>> >>> On Fri, May 5, 2017 at 3:18 PM, Daniel Dekany <[email protected]> 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! >>> 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 > > Any reaction to this? Right now I have no better idea than disallowing > calling build() for more than once (by throwing IllegalStateException).
Sorry for late response. You're right. It seems best to throw an exception in favor of immutability. Regards, Woonsan > >>> 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 <[email protected]> >>>>> 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 <[email protected]> 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 >
