Curious: how long do you think it would take you to convert all plugins? Gary
On Mon, Jun 16, 2014 at 11:06 AM, Matt Sicker <[email protected]> wrote: > Paul, that's almost exactly what I was suggesting with the builders! I've > only converted two so far as demos to see what everyone thought about doing > it that way. If we wanted to wait until 2.1 or something to do a change > like that, I'd be fine with that. I could spend the time converting them > all which would also give me a nice overview of all the existing plugins. > > > On 16 June 2014 08:49, Paul Benedict <[email protected]> wrote: > >> I don't know why the Spring link isn't working (try later) but this is >> how I think you guys can make the builder pattern better. Honestly, it's >> impossible to comprehend what the parameters mean just by looking at the >> code in the current form. >> >> Example returning a custom builder interface: >> FileAppender.builder() >> .setPropertyX(true) >> .setPropertyY(true) >> .setPropertyZ(4096) >> .setPropertyA(null) >> .create(); >> >> Paul >> >> >> >> Cheers, >> Paul >> >> >> On Mon, Jun 16, 2014 at 8:41 AM, Remko Popma <[email protected]> >> 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 <[email protected]> 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 <[email protected]> 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 < >>>>> [email protected]> 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 <[email protected]> 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 <[email protected]> wrote: >>>>> >>>>> I'm fine with just the factory methods too. >>>>> >>>>> Sent from my iPhone >>>>> >>>>> On 2014/06/16, at 9:44, Scott Deboy <[email protected]> wrote: >>>>> >>>>> +1 >>>>> On Jun 15, 2014 4:05 PM, "Ralph Goers" <[email protected]> >>>>> 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 <[email protected]> >>>>> 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 <[email protected]> >>>>> 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 <[email protected]> >>>> >>> >>> >> > > > -- > Matt Sicker <[email protected]> > -- E-Mail: [email protected] | [email protected] Java Persistence with Hibernate, Second Edition <http://www.manning.com/bauer3/> JUnit in Action, Second Edition <http://www.manning.com/tahchiev/> Spring Batch in Action <http://www.manning.com/templier/> Blog: http://garygregory.wordpress.com Home: http://garygregory.com/ Tweet! http://twitter.com/GaryGregory
