Also, are tests that were @Ignore'd last week still broken?
Gary
Original message From: Remko Popma
Date:06/16/2014 02:28 (GMT-05:00)
To: Log4J Developers List
Subject: Re: Build failures again
I'm fairly sure I ran the build and it worked after my last commit.
Sent fr
No I fixed the problem and un ignored the tests.
Date: Sun Jun 15 06:40:04 2014
New Revision: 1602666
Sent from my iPhone
> On 2014/06/16, at 19:43, Gary Gregory wrote:
>
> Also, are tests that were @Ignore'd last week still broken?
>
> Gary
>
>
> Original message
> Fro
Great, thank you.
Gary
On Mon, Jun 16, 2014 at 7:19 AM, Remko Popma wrote:
> No I fixed the problem and un ignored the tests.
>
> Date: Sun Jun 15 06:40:04 2014
> New Revision: 1602666
>
> Sent from my iPhone
>
> On 2014/06/16, at 19:43, Gary Gregory wrote:
>
> Also, are tests that were @Ign
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 wrote:
> ...and another (shorter) way would be to add a comment for each parameter:
> factoryMethod(
> null, // loggerName
> null, // Marker
> null, // Log
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 prod
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
This might be related to a bug fix patch I committed last night. I'll have
a look when I get to work.
On Monday, 16 June 2014, Ralph Goers wrote:
>
> Failed tests:
> TestConfigurator.testReconfiguration:236 Configuration not reset
> FileConfigTest.testReconfiguration:62 Reconfiguration faile
I am seeing the same problems with the latest updates from trunk:
On Mon, Jun 16, 2014 at 1:26 AM, Ralph Goers
wrote:
>
> Failed tests:
> TestConfigurator.testReconfiguration:236 Configuration not reset
> FileConfigTest.testReconfiguration:62 Reconfiguration failed expected
> not same
>
On Mon, Jun 16, 2014 at 9:37 AM, Gary Gregory
wrote:
> I am seeing the same problems with the latest updates from trunk:
>
Failed tests:
TestConfigurator.testReconfiguration:236 Configuration not reset
FileConfigTest.testReconfiguration:62 Reconfiguration failed expected not
same
LoggerTes
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 rem
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:
FileAppen
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
that
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 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 of the builder.se
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 fo
Of course. Yes, create() is supposed to validate.
Cheers,
Paul
On Mon, Jun 16, 2014 at 9:26 AM, Gary Gregory
wrote:
> On Mon, Jun 16, 2014 at 10:23 AM, Remko Popma
> wrote:
>
>> Gary, good question.
>> To be honest, I'm not that convinced that builders are by definition
>> better.
>> For exa
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 t
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 wrote:
> It's much more than personal preference. It's about readability and
> maintainability. Think about what will happen if you have to add ano
Which is a runtime error, not a compile time error.
Ralph
On Jun 16, 2014, at 7:27 AM, Paul Benedict wrote:
> Of course. Yes, create() is supposed to validate.
>
>
> Cheers,
> Paul
>
>
> On Mon, Jun 16, 2014 at 9:26 AM, Gary Gregory wrote:
> On Mon, Jun 16, 2014 at 10:23 AM, Remko Popma
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 possib
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 wrote:
> About the validation, wasn't
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 wrote:
> Paul this is about the plugin factory methods that are called by the
> PluginManager to turn a configuration text file int
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 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 doing
> it that
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 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 less. I can generate
I vote for setFoo()
Cheers,
Paul
On Mon, Jun 16, 2014 at 10:16 AM, Matt Sicker wrote:
> On 16 June 2014 10:11, Gary Gregory 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
> sche
> On 2014/06/16, at 23:30, Ralph Goers 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 user created
> plugin
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 map)
might make more sense. Then it wouldn’t much matter
On Tuesday, June 17, 2014, Ralph Goers 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 createPlugin(PluginType type, Map map)
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 o
> On 2014/06/17, at 0:40, Ralph Goers 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, such
> as a configurat
On Mon, Jun 16, 2014 at 12:04 PM, Remko Popma wrote:
>
> On 2014/06/17, at 0:40, Ralph Goers 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 o
Might I point out items 1 and 2 in Effective Java?
On 16 June 2014 11:14, Gary Gregory wrote:
> On Mon, Jun 16, 2014 at 12:04 PM, Remko Popma
> wrote:
>
>>
>> On 2014/06/17, at 0:40, Ralph Goers wrote:
>>
>> From the Map perspective it may not be better. But from the perspective
>> of abstrac
Robert Veitch created LOG4J2-671:
Summary: Property using ${date:} is evaluated multiple times
across appenders
Key: LOG4J2-671
URL: https://issues.apache.org/jira/browse/LOG4J2-671
Project: Log4j 2
SIBISH BASHEER created LOG4J2-672:
-
Summary: AsyncLogger logs missing
Key: LOG4J2-672
URL: https://issues.apache.org/jira/browse/LOG4J2-672
Project: Log4j 2
Issue Type: Bug
Componen
Yes, this is another case where simply running the unit tests would have shown
that the fix for LOG4J2-619 was bad as it caused all reconfigurations to fail.
Please stop being so quick to commit without testing.
I fixed the problem.
Ralph
On Jun 16, 2014, at 6:27 AM, Matt Sicker wrote:
>
[
https://issues.apache.org/jira/browse/LOG4J2-619?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14033410#comment-14033410
]
Ralph Goers commented on LOG4J2-619:
The fix was bad. It checked for a rootElement be
Oh wow, I can't believe I missed that... Could have sworn I ran the tests,
too. Guess not.
On 16 June 2014 22:55, Ralph Goers wrote:
> Yes, this is another case where simply running the unit tests would have
> shown that the fix for LOG4J2-619 was bad as it caused all reconfigurations
> to fail
44 matches
Mail list logo
