logging-log4j-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Ralph Goers <ralph.go...@dslextreme.com>
Subject Re: debug output
Date Mon, 16 Jun 2014 15:26:23 GMT
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 <boards@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 <garydgregory@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.popma@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 <pbenedict@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.popma@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.goers@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 <pbenedict@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.popma@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 <boards@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 <pbenedict@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.goers@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 <boards@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.popma@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.deboy@gmail.com> wrote:
>>> 
>>>> +1
>>>> 
>>>> On Jun 15, 2014 4:05 PM, "Ralph Goers" <ralph.goers@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.popma@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.goers@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 <boards@gmail.com>
>> 
>> 
> 
> 
> 
> 
> 
> 
> 
> -- 
> E-Mail: garydgregory@gmail.com | ggregory@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 <boards@gmail.com>


Mime
View raw message