> On 2014/06/16, at 23:30, Ralph Goers <ralph.go...@dslextreme.com> wrote: > > As it is now, the code supports both builders and factory methods. If it was > left that way then things could be converted over time. But I am concerned > about users. We currently document using factory methods so any user created > plugins are doing that. If we switch to builders I would expect the code that > supports factory methods would want to be removed so user plugins with > factory methods would have to be converted.
That's a good point, I hadn't thought of that. > > Ralph > >> On Jun 16, 2014, at 7:15 AM, Gary Gregory <garydgreg...@gmail.com> wrote: >> >> FWIW, it does seem like a *lot* of work to redo some if not all plugins and >> I do understand Matt's POV. Would it even be possible to add builders later? >> Would we have to keep factory methods around for BC? I do not think there is >> a right and wrong pattern here, it's just that we have something that works >> now, indeed, with some pains here and there (default values for example). >> >> Here is an alternate question: if we could wave a magic wand and get the >> work done now, which solution would we want? >> >> Gary >> >> >> >>> On Mon, Jun 16, 2014 at 9:41 AM, Remko Popma <remko.po...@gmail.com> wrote: >>> Matt, >>> >>> Ralph's argument (and I agree) was that we want only one way to do plugins, >>> either with a factory method or with a builder. >>> >>> Currently only two plugins use the new builder: PatternLayout and >>> HtmlLayout. >>> So we can either revert these two back to use a factory method, or we can >>> convert the remaining 147 uses of the factory method to builders. >>> That last option would mean writing the builders for those plugins, and >>> modifying all JUnit tests that currently use the factory methods. >>> >>> I agree with Ralph that converting all remaining 147 places to use builders >>> would take up a lot of our time and energy for very little gain. >>> >>> Matt, you mentioned that you could live with factory methods if there was a >>> better way to write JUnit tests than >>> FileAppender.createAppender("true", "true", "true", "true", "true", "true", >>> "true", "4096", null, null, "false", null, null); >>> >>> I believe I've demonstrated several ways in which such JUnit test code can >>> be improved. >>> Are you okay with removing the builders and reverting back to the factory >>> methods? >>> >>> >>> >>>> On Mon, Jun 16, 2014 at 10:23 PM, Matt Sicker <boa...@gmail.com> wrote: >>>> I'm unable to load that page for some reason, but yes, using a factory >>>> method as such is really not extensible. If the factory method took a Map >>>> or some sort of configuration object (which itself could use the fluent >>>> builder pattern), that would be less tightly coupled. >>>> >>>> Wouldn't it also make sense for the factory methods to be private or >>>> similar? That way they're only accessible through reflection. >>>> >>>> >>>>> On Sunday, 15 June 2014, Paul Benedict <pbened...@apache.org> wrote: >>>>> You don't want factory methods that take umpteen arguments. That's no way >>>>> to make your builder extensible for future enhancements. Instead, you >>>>> want a builder object that has a fluent API. >>>>> >>>>> http://docs.spring.io/spring/docs/3.2.8.RELEASE/javadoc-api/org/springframework/beans/factory/support/BeanDefinitionBuilder.html >>>>> >>>>> >>>>> Cheers, >>>>> Paul >>>>> >>>>> >>>>> On Sun, Jun 15, 2014 at 10:04 PM, Ralph Goers >>>>> <ralph.go...@dslextreme.com> wrote: >>>>> Matt, >>>>> >>>>> The only objection I have to builders is that there should only be one >>>>> way to configure plugins and I have neither the time or energy to convert >>>>> all plugins from factories to builders. With 130+ open issues I think our >>>>> time is better focused there instead of fixing something that already >>>>> works. >>>>> >>>>> And, FWIW, the only place you will see createAppender coded like that is >>>>> in unit tests and there are a bunch of ways to make that clearer. >>>>> >>>>> Ralph >>>>> >>>>>> On Jun 15, 2014, at 7:19 PM, Matt Sicker <boa...@gmail.com> wrote: >>>>>> >>>>>> I'm really against using factory methods due to language limitations in >>>>>> Java. You can't specify default values, for one. Two, the more >>>>>> parameters a factory takes, the crazier the method is. Seriously, tell >>>>>> me what this method is specifying: >>>>>> >>>>>> FileAppender.createAppender("true", "true", "true", "true", "true", >>>>>> "true", "true", "4096", null, null, "false", null, null); >>>>>> >>>>>> >>>>>> On 15 June 2014 21:05, Remko Popma <remko.po...@gmail.com> wrote: >>>>>> I'm fine with just the factory methods too. >>>>>> >>>>>> Sent from my iPhone >>>>>> >>>>>>> On 2014/06/16, at 9:44, Scott Deboy <scott.de...@gmail.com> wrote: >>>>>>> >>>>>>> +1 >>>>>>> >>>>>>> On Jun 15, 2014 4:05 PM, "Ralph Goers" <ralph.go...@dslextreme.com> >>>>>>> wrote: >>>>>>> Do we need the builders? As I said, I prefer only one way for creating >>>>>>> plugins. >>>>>>> >>>>>>> Ralph >>>>>>> >>>>>>>> On Jun 15, 2014, at 2:49 PM, Remko Popma <remko.po...@gmail.com> wrote: >>>>>>>> >>>>>>>> I see. I agree that the original format is much nicer. >>>>>>>> >>>>>>>> Matt, do you think you can achieve this with the builders? >>>>>>>> >>>>>>>> Sent from my iPhone >>>>>>>> >>>>>>>>> On 2014/06/16, at 1:29, Ralph Goers <ralph.go...@dslextreme.com> >>>>>>>>> wrote: >>>>>>>>> >>>>>>>>> While you improved some of the existing messages, you really didm’t >>>>>>>>> address what I wanted fixed. The previous debug logs would have had >>>>>>>>> messages similar to: >>>>>>>>> >>>>>>>>> Calling createLayout on class >>>>>>>>> org.apache.logging.log4j.core.layout.PatternLayout for element >>>>>>>>> PatternLayout with params(pattern="%d{HH:mm:ss.SSS} [%t] %-5level >>>>>>>>> %logger{36} - %msg%n", >>>>>>>>> Configuration(D:\rista\eclipsekws\.metadata\.plugins\org.eclipse.wst.server.core\tmp1\wtpwebapps\log4j2.0-test\WEB-INF\classes\test-log4j.xml), >>>>>>>>> null, charset="null", alwaysWriteExceptions="null") >>>>>>>>> >>>>>>>>> Calling createAppender on class >>>>>>>>> org.apache.logging.log4j.core.appender.ConsoleAppender for element >>>>>>>>> Console with params(PatternLayout(%d{HH:mm:ss.SSS} [%t] %-5level >>>>>>>>> %logger{36} - %msg%n), null, target="SYSTEM_OUT", name="console", >>>>>>>>> follow="null", ignoreExceptions="null") >>>>>>>>> >>>>>>>>> Calling createAppenderRef on class >>>>>>>>> org.apache.logging.log4j.core.config.AppenderRef for element >>>>>>>>> appender-ref with params(ref="console", level="null", null) >>>> >>>> >>>> -- >>>> Matt Sicker <boa...@gmail.com> >> >> >> >> -- >> E-Mail: garydgreg...@gmail.com | ggreg...@apache.org >> Java Persistence with Hibernate, Second Edition >> JUnit in Action, Second Edition >> Spring Batch in Action >> Blog: http://garygregory.wordpress.com >> Home: http://garygregory.com/ >> Tweet! http://twitter.com/GaryGregory >