Right, so they are not part of the public API, unless we consider programmatic configuration (not from a file).
Gary On Mon, Jun 16, 2014 at 10:56 AM, Remko Popma <remko.po...@gmail.com> wrote: > Paul this is about the plugin factory methods that are called by the > PluginManager to turn a configuration text file into an object hierarchy > with Loggers, Appenders, etc. > It would be very rare for client code to call these methods directly. > Which is why they are primarily called from JUnit tests. > > > On Mon, Jun 16, 2014 at 11:45 PM, Paul Benedict <pbened...@apache.org> > wrote: > >> If this API is primarily for unit tests, then I don't care as much. I >> thought this was Public API. When it comes to Public API, I prefer to make >> it as friendly-to-read and maintainable as possible. >> >> >> Cheers, >> Paul >> >> >> On Mon, Jun 16, 2014 at 9:42 AM, Remko Popma <remko.po...@gmail.com> >> wrote: >> >>> About the validation, wasn't the argument that some of the current JUnit >>> tests look like this? >>> FileAppender.createAppender("true", "true", "true", "true", "true", >>> "true", "true", "4096", null, null, "false", null, null); >>> >>> So, null is fine here for at least four arguments. >>> So it is definitely possible to forget to call the >>> builder.setValue(value) method for those arguments >>> - which is fine if your intention was to set null, but not if you >>> forgot to set some non-null value. >>> >>> The compiler validates that you have the right number of arguments for >>> free. >>> It is not possible for builders to provide the same guarantee unless all >>> arguments must be non-null. >>> >>> About the readability - and as Ralph pointed out this is readability in >>> JUnit code primarily - there are a number of ways to accomplish that. >>> >>> >>> >>> On Mon, Jun 16, 2014 at 11:27 PM, Ralph Goers < >>> ralph.go...@dslextreme.com> wrote: >>> >>>> That is pretty much what Matt implemented. See the other arguments >>>> around this, the primary one being that the factory methods are normally >>>> invoked dynamically so you wouldn’t be looking at the code that calls them. >>>> They are primarily called in the open in unit tests. >>>> >>>> Ralph >>>> >>>> On Jun 16, 2014, at 6:49 AM, Paul Benedict <pbened...@apache.org> >>>> 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 <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 <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