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>
>>>>>
>>>>
>>>>
>>>
>>>
>>
>
