Yes, for a programmatic configuration builders would be better. Unless, of
course, we came up with some other way for programmatic configurations to
interact with plugins. For example, a method like
T createPlugin(PluginType type, Map<String, Object> map)
might make more sense. Then it wouldn’t much matter whether we are using
builders or the factory pattern.
Ralph
On Jun 16, 2014, at 8:12 AM, Matt Sicker <boa...@gmail.com> wrote:
> Programmatic configuration is feature in that list of issues Ralph mentioned.
> I don't see any good way to do that without using the fluent builder pattern
> as that's how programmatic configuration APIs are usually done. The other
> ways are usually things like closures/lambdas, but that would require Java
> 1.8+ to work. Groovy supports closures which is what makes Gradle so much
> less verbose, so that, too, is an alternative way to support builders.
>
> I guess the important thing to consider here is that the builders aren't for
> _plugins_, but are for plugin _configurations_. That makes me think that the
> factory methods (whether they be private or public) should be far more
> generic such as accepting a PluginConfiguration object (each specific to the
> plugin) or a generic Map<String, Object> that also makes it a lot simpler to
> pass in plugin attributes.
>
>
> On 16 June 2014 10:05, Gary Gregory <garydgreg...@gmail.com> wrote:
> 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
> JUnit in Action, Second Edition
> Spring Batch in Action
> Blog: http://garygregory.wordpress.com
> Home: http://garygregory.com/
> Tweet! http://twitter.com/GaryGregory
>
>
>
> --
> Matt Sicker <boa...@gmail.com>
