On Fri, Feb 17, 2017 at 4:09 PM, Daniel Dekany <[email protected]> wrote: > Friday, February 17, 2017, 3:16:28 PM, Woonsan Ko wrote: > >> On Fri, Feb 17, 2017 at 4:22 AM, Daniel Dekany <[email protected]> wrote: >>> Friday, February 17, 2017, 6:52:15 AM, Woonsan Ko wrote: >>> >>>> On Thu, Feb 16, 2017 at 1:28 PM, Daniel Dekany <[email protected]> wrote: >>>>> One other problem I see with builders is that they seem to be >>>>> difficult to work with when it comes to working out the Configuration >>>>> setting values in multiple stages. >>>>> >>>>> What do I mean by multiple stages? For example, the user specifies >>>>> some settings in the web.xml in the case of FreemarkeServlet, so >>>>> applying those is stage one. Then the Configuration builder is worked >>>>> on by FreemarkeServlet itself, which adjusts it further, usually by >>>>> setting some defaults where the setting wasn't set in the previous >>>>> stage. Then it goes down one more stage, to FreeMarker itself, where >>>>> the FreeMarker defined defaults are applied to the settings which were >>>>> not set so far. You may say that it should be done on the opposite >>>>> order, that is, FreeMarker fills its defaults, then FreemarkerServlet >>>>> overwrites some setting with its own defaults, then comes the web.xml >>>>> content. That's simpler to do, but has some problems. One is that you >>>>> may construct setting values that will be later overwritten, and not >>>>> all setting values are cheap to construct (unless... for complex value >>>>> we require passing in a builder instead of the object). The other is >>>> >>>> Another option I'm thinking is to extend the Builder pattern to be >>>> able to use an existing object instead of new creation when needed. >>>> As an example pseudo code, >>>> >>>> // stage #1: >>>> Configuration config = Config,Builder.create(props) // create new >>>> .templateUpdateDelayMilliseconds(60000) >>>> .defaultEncoding("UTF-8") >>>> //... >>>> .build(); >>>> >>>> // stage #2: >>>> config = Config.Builder.update(config) // update existing config >>>> .whitespaceStripping(true) >>>> //... >>>> .build(); // return the same config instance after updates >>>> >>>> // stage #n: >>>> // ... >>>> >>>>> that you may want to adjust settings nested inside values (such as the >>>>> settings of an ObjectWrapper), so the top-level setting must be >>>>> already set by the higher layer when it gets to you. Anyway, whichever >>>>> order you chose, this is the use case. >>>> >>>> Nested one can also create or update (or createOrUpdate sometimes?), IMHO: >>>> >>>> // stage #1: >>>> Configuration config = Config,Builder.create(props) >>>> .objectWrapperBuilder( >>>> // new builder to create a new objectWrapper >>>> DefaultObjectWrapper.Builder.create() >>>> .iterableSupport(true) >>>> //... >>>> .build() >>>> ) >>>> //... >>>> .build(); >>>> >>>> // stage #2: >>>> config = Config.Builder.update(config) >>>> .objectWrapperBuilder( >>>> // new builder to update existing objectWrapper >>>> >>>> DefaultObjectWrapper.Builder.update(config.getObjectWrapper()) >>>> .forceLegacyNonListCollections(true) >>>> //... >>>> .build() >>>> ) >>>> //... >>>> .build(); >>> >>> That (recreate Builder from the built object) is a possibility. But >>> note that it comes with some extra price over the obvious extra >>> complexity it adds: >>> >>> - Objects are fully built, then dropped, then rebuilt... It almost never >>> matters in out case, but creating some setting values, like creating >>> some custom TemplateLoader-s or ObjectWrapper-s, can be relatively >>> expensive (like it involves I/O or permanent allocation of bigger >>> static data structures). So it's not an universally applicable >>> pattern at least. >> >> Oh, that's not what I meant. >> In the first example, >> >> // stage #2: >> config = Config.Builder.update(config) // update existing config >> .whitespaceStripping(true) >> //... >> .build(); // return the same config instance after updates >> >> the first line takes an existing Configuration object only to update >> and return it again only after setting more properties. >> So, the return is meaningless here, and #build() means 'updating >> build', not 'creation build', which I think can be regarded as an >> extended Builder pattern. > [snip] > > 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? 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? > >> Yeah, I'm fine with `T getFoo()' or `boolean isFoo()'. But if it >> follows a fluent style, it's already free from the JavaBeans >> conventions anyway. > > Many frameworks/tools look for JavaBean properties. Like Spring IoC > for example, if you are using the XML configuration. They couldn't > get/set the properties through a fluent API. I have assumed freemarker-spring layer on top of the core API including builders. freemarker-spring could follow JavaBean properties. > >> The exception in fluent Builders seems fine to me. > > (What exception do you mean?) Just allowing fluent style for Builders in the core api, not JavaBeans style. That's what I meant by 'exception'. Woonsan > >> Regards, >> >> Woonsan >> >>> >>>> Kind regards, >>>> >>>> Woonsan >>>> >>>>> >>>>> Any thoughts? Especially on what to do with those nested settings. >>>>> >>>>> >>>>> Wednesday, February 15, 2017, 8:54:52 PM, Woonsan Ko wrote: >>>>> >>>>>> On Wed, Feb 15, 2017 at 5:48 AM, Daniel Dekany <[email protected]> >>>>>> wrote: >>>>>>> Wednesday, February 15, 2017, 12:16:12 AM, Woonsan Ko wrote: >>>>>>> >>>>>>>> Also, if possible, I would like to have an immutable object or >>>>>>>> representation than the Configuration itself. For example, >>>>>>>> template#getConfiguration doesn't have to return a mutable object, for >>>>>>>> instance. But it requires a bigger change, I guess. >>>>>>> >>>>>>> A good point, that's the totally correct way of doing things. So we >>>>>>> could have a mutable Configuration.Builder, which yields an immutable >>>>>>> Configuration. We also have some ObjectWrapper, which is a bean in >>>>>>> itself, but is also part of the Configuration. We certainly will have >>>>>>> another such "injected bean", the TemplateResolver, and maybe some >>>>>>> more on long term. Those also could follow this pattern (i.e, being >>>>>>> immutable and only offer a builder). >>>>>>> >>>>>>> Question is, what extra pain that means for the average user. Like >>>>>>> then in Spring XML you have to create a factory bean from Xxx.Builder, >>>>>>> and then create yet another bean with factory-bean and factory-method >>>>>>> attribute (because for some reason javax.annotation doesn't have >>>>>>> anything like Spring FactoryBean, and freemarker-core can't depend on >>>>>>> Spring obviously). Or, we can subclass all builders in something like >>>>>>> freemarker-spring, to create a FactoryBean version of them, and then >>>>>>> tell people to (a) pull in the depednecy and (b) use those classes >>>>>>> instead of those shown in the core JavaDoc and in the Manual >>>>>>> examples... So it's not very streamlined. If you have traditional >>>>>>> beans, it's simpler for the users (especially if they don't have a >>>>>>> PostConstruct either). Note sure which compromise is the winner. >>>>>> >>>>>> From spring users' perspective, I don't think it would be a problem >>>>>> for them to add one more dependency on freemarker-spring to use the >>>>>> factory beans (dedicatedly designed for spring). And, they have >>>>>> already been doing it with others (JndiObjectFactoryBean, >>>>>> MBeanServerFactoryBean, FreemarkerConfigurationFactoryBean, etc.). >>>>>> It might be a little bit more work from development perspective, but I >>>>>> don't see anything weird from end users' perspective by having >>>>>> freemarker-spring and FactobyBean layers. >>>>>> Also, in general, I think it's a lot safer and more resilient in >>>>>> upgrade paths to use a factory bean instead of allowing to access >>>>>> mutable bean directly because we can safely control everything through >>>>>> the factory. >>>>>> >>>>>> Anyway, I like the idea having Builders which could perhaps look like >>>>>> a fluent Java DSL. For example, >>>>>> >>>>>> Configuration config = ConfigBuilder.create(props) // ConfigBuilder >>>>>> or Config.Builder >>>>>> .templateUpdateDelayMilliseconds(60000) >>>>>> .defaultEncoding("UTF-8") >>>>>> //... >>>>>> .build(); >>>>>> >>>>>> I think this will help people using Java API directly. And, I think >>>>>> it's also a good idea to have FactoryBeans (using the Builder APIs) in >>>>>> freemarker-spring for their convenience. >>>>>> >>>>>>> >>>>>>> If Configuration (and the others) remains a bean, I want to throw an >>>>>>> IllegalStateException if a the object were already "used" (like >>>>>>> template processing was already invoked), and then someone calls >>>>>>> cfg.setXxx(value). In FM2 we say that if you do that from multiple >>>>>>> threads, then it won't work correctly, but we allow changing the >>>>>>> Configuration at any time if you are only using it from the same >>>>>>> thread. That's a possibility I want to remove, even if we don't switch >>>>>>> to builders. >>>>>> >>>>>> Runtime exception option sounds like less attractive to me than >>>>>> factory beans. I would happily accept freemarker-spring library to >>>>>> take advantage of all the good factory beans. >>>>>> >>>>>>> >>>>>>> Oh, and yet another complication with builders... If Configuration is >>>>>>> immutable, then it only will have getters, no setters. But, >>>>>>> Environment is mutable with setters, and it should be. Both >>>>>>> Configuration and Environment extends Configurable. So it seems now we >>>>>>> should split that class into a getters-only class and a subclass that >>>>>>> adds setters... (That might will be necessary because that Template is >>>>>>> mutable with setters is not all right either. Or we can just throw >>>>>>> IllegalStateException there too.) >>>>>> >>>>>> Yeah, I would separate those. >>>>>> >>>>>> Regards, >>>>>> >>>>>> Woonsan >>>>>> >>>>>>> >>>>>>> -- >>>>>>> Thanks, >>>>>>> Daniel Dekany >>>>>>> >>>>>> >>>>> >>>>> -- >>>>> Thanks, >>>>> Daniel Dekany >>>>> >>>> >>> >>> -- >>> Thanks, >>> Daniel Dekany >>> >> > > -- > Thanks, > Daniel Dekany >
