One very simple way I use to clarify which parameter is what is to declare
a local variable for each param.
For example:
(and this is not strictly a @Plugin factory method, but you get my point...)
public class RingBufferLogEventTest {
@Test
public void testGetLevelReturnsOffIfNullLevelSet() {
...and another (shorter) way would be to add a comment for each parameter:
factoryMethod(
null, // loggerName
null, // Marker
null, // LoggerFQCN
null, // Level
null, // Message
null, // Throwable
null, // ContextMap
null, // ContextStack
null, // ThreadName
null, // Location
(I'm talking about doing JUnit tests on the factory methods in case that
was not clear.)
On Mon, Jun 16, 2014 at 9:42 PM, Remko Popma remko.po...@gmail.com wrote:
...and another (shorter) way would be to add a comment for each parameter:
factoryMethod(
null, // loggerName
null, //
I thought of an interesting work around in unit tests by using the
PluginBuilder directly by setting up a Node object manually. This seemed
more clear because it requires using the named parameters. I could probably
work on a fluent interface to using that.
Also, we use the factory methods in
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
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
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:
FWIW, it does seem like a *lot* of work to redo some if not all plugins and
I do understand Matt's POV. Would it even be possible to add builders
later? Would we have to keep factory methods around for BC? I do not think
there is a right and wrong pattern here, it's just that we have something
Gary, good question.
To be honest, I'm not that convinced that builders are by definition better.
For example, if you miss a parameter with the factory method, you get a
compilation error.
If you forget to call one of the builder.setValue(value) methods, you may
never notice...
I see the argument
On Mon, Jun 16, 2014 at 10:23 AM, Remko Popma remko.po...@gmail.com wrote:
Gary, good question.
To be honest, I'm not that convinced that builders are by definition
better.
For example, if you miss a parameter with the factory method, you get a
compilation error.
If you forget to call one
It's much more than personal preference. It's about readability and
maintainability. Think about what will happen if you have to add another
option to FileAppender: you have to create another overloaded method with
another argument. This pattern will continue and will not get any easier.
It also
Of course. Yes, create() is supposed to validate.
Cheers,
Paul
On Mon, Jun 16, 2014 at 9:26 AM, Gary Gregory garydgreg...@gmail.com
wrote:
On Mon, Jun 16, 2014 at 10:23 AM, Remko Popma remko.po...@gmail.com
wrote:
Gary, good question.
To be honest, I'm not that convinced that builders
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,
As it is now, the code supports both builders and factory methods. If it was
left that way then things could be converted over time. But I am concerned
about users. We currently document using factory methods so any user created
plugins are doing that. If we switch to builders I would expect
No. You add the parameter to the factory method and modify any unit tests that
fail to compile.
Ralph
On Jun 16, 2014, at 7:27 AM, Paul Benedict pbened...@apache.org wrote:
It's much more than personal preference. It's about readability and
maintainability. Think about what will happen if
Which is a runtime error, not a compile time error.
Ralph
On Jun 16, 2014, at 7:27 AM, Paul Benedict pbened...@apache.org wrote:
Of course. Yes, create() is supposed to validate.
Cheers,
Paul
On Mon, Jun 16, 2014 at 9:26 AM, Gary Gregory garydgreg...@gmail.com wrote:
On Mon, Jun
Hit enter too soon.
Of course, the configuration is only processed at runtime so any parameters
missing in the configuration are going to be detected at runtime regardless of
which way it is done. Compile vs runtime is only a benefit to the factory
methods on unit tests (which is ironic since
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
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
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.
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
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
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 boa...@gmail.com 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
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
On 16 June 2014 10:11, Gary Gregory garydgreg...@gmail.com wrote:
Curious: how long do you think it would take you to convert all plugins?
Longer than I'd like to wait for 2.0 to be released, but in the grand
scheme of things, I think I could probably crank it out over like six
weekends. Maybe
I vote for setFoo()
Cheers,
Paul
On Mon, Jun 16, 2014 at 10:16 AM, Matt Sicker boa...@gmail.com wrote:
On 16 June 2014 10:11, Gary Gregory garydgreg...@gmail.com wrote:
Curious: how long do you think it would take you to convert all plugins?
Longer than I'd like to wait for 2.0 to be
On 2014/06/16, at 23:30, Ralph Goers ralph.go...@dslextreme.com wrote:
As it is now, the code supports both builders and factory methods. If it was
left that way then things could be converted over time. But I am concerned
about users. We currently document using factory methods so any
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, MapString, Object map)
might make more sense. Then it
On Tuesday, June 17, 2014, Ralph Goers ralph.go...@dslextreme.com wrote:
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
From the Map perspective it may not be better. But from the perspective of
abstracting how individual Plugins are created it probably is better.
Remember, this is for allowing some sort of programmatic configuration, such as
a configuration file written in Groovy. I find the idea of that kind
On 2014/06/17, at 0:40, Ralph Goers ralph.go...@dslextreme.com wrote:
From the Map perspective it may not be better. But from the perspective of
abstracting how individual Plugins are created it probably is better.
Remember, this is for allowing some sort of programmatic configuration,
On Mon, Jun 16, 2014 at 12:04 PM, Remko Popma remko.po...@gmail.com wrote:
On 2014/06/17, at 0:40, Ralph Goers ralph.go...@dslextreme.com wrote:
From the Map perspective it may not be better. But from the perspective of
abstracting how individual Plugins are created it probably is better.
Might I point out items 1 and 2 in Effective Java?
On 16 June 2014 11:14, Gary Gregory garydgreg...@gmail.com wrote:
On Mon, Jun 16, 2014 at 12:04 PM, Remko Popma remko.po...@gmail.com
wrote:
On 2014/06/17, at 0:40, Ralph Goers ralph.go...@dslextreme.com wrote:
From the Map perspective
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
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
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
Well, it could be done via the builders. The visit method just needs to
accept a StringBuilder to continue building the log message. I'll take a
look at this shortly.
And in regards to builders versus factories, I actually have a third option
that could reduce everything: prototypes. Instead of
@logging.apache.org
/divdivSubject: Re: debug output /divdiv
/divDo 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
Original message
From: Ralph Goers
Date:06/15/2014 19:04 (GMT-05:00)
To: Log4J Developers List
Subject: Re: debug output
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
+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.
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.
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,
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
/divdivDate:06/15/2014 19:31 (GMT-05:00)
/divdivTo: Log4J Developers List log4j-dev@logging.apache.org
/divdivSubject: Re: debug output /divdiv
/divWell, it could be done via the builders. The visit method just needs to
accept a StringBuilder to continue building the log message. I'll take a look
Well, I can live with factory methods if we have a better method for doing
unit tests. Not all unit tests need an XML configuration file.
On 15 June 2014 22:04, Ralph Goers ralph.go...@dslextreme.com wrote:
Matt,
The only objection I have to builders is that there should only be one way
to
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.
I've done some work on this, there may be more places to improve, I mainly
focused on PluginBuilder and PluginAttributeVisitor.
On Thu, Jun 12, 2014 at 2:09 AM, Matt Sicker boa...@gmail.com wrote:
Yeah, I liked the prettier logging format. I was planning to add it back
in, but it appears as
Yeah, I liked the prettier logging format. I was planning to add it back
in, but it appears as though I completely forgot about it! The new format
was a quick placeholder. I'll try and work on that this week.
On 10 June 2014 19:47, Gary Gregory garydgreg...@gmail.com wrote:
Maybe Matt can shed
On Tue, Jun 10, 2014 at 7:11 PM, Ralph Goers ralph.go...@dslextreme.com
wrote:
I am working on a new Appender and am noticing that the debug output is
now far less useful than it used to be. I used to see the factory method
being invoked with all of its parameters very nicely formatted. Now I
I think the discussion was not on its own thread. I thought there was
agreement that there should be only one way to configure plugins. I prefer the
factory method simply because it would be a whole lot of effort to convert
everything to a builder and I just don't think the benefit is worth
Well, for Log4j Plugins, one way to configure should be plenty. I am OK
with the factory method pattern, while it makes for some long signatures, I
like that it is all in one place.
May I suggest a simple -1 reply on the ML on the changes to logging? Do
you feel a VETO is inappropriate here?
I don't know exactly what I would be vetoing. I have no problem with some of
the refactoring. The commit(s) that changed the logging probably happened weeks
ago and I am just noticing now.
But yes, I want the logging aspect of the changes reverted back to what was
previously being done.
Sent
Maybe Matt can shed a light on this?
Gary
On Tue, Jun 10, 2014 at 8:43 PM, Ralph Goers rgo...@apache.org wrote:
I don't know exactly what I would be vetoing. I have no problem with some
of the refactoring. The commit(s) that changed the logging probably
happened weeks ago and I am just
53 matches
Mail list logo
