Definitely will add the tests :)
I did notice however that the ThrowableProxy does use the hard coded "\n" throughout, however, in all *ThrowablePatternConverter's, it re-splits them on "\n" when a line limit is specified: http://svn.apache.org/viewvc/logging/log4j/log4j2/trunk/core/src/main/java/org/apache/logging/log4j/core/pattern/ExtendedThrowablePatternConverter.java?view=markup http://svn.apache.org/viewvc/logging/log4j/log4j2/trunk/core/src/main/java/org/apache/logging/log4j/core/pattern/RootThrowablePatternConverter.java?view=markup http://svn.apache.org/viewvc/logging/log4j/log4j2/trunk/core/src/main/java/org/apache/logging/log4j/core/pattern/ThrowablePatternConverter.java?view=markup I guess there are two approaches, here: 1. Update ThrowableProxy.java to have a separator provided. 2. Always run that bit of code if the separator differs from the default, for instance: ExtendedThrowablePatternConverter.java:110: if (lines != Integer.MAX_VALUE && !Constants.LINE_SEP.equals(this.stackTraceSeparator)) { Seems #1 would be most efficient but more complex? I'll do #1 as you had originally suggested. I did have a few questions on your preferred implementation: 1. Update the ThrowableProxy getExtendedStackTrace(List<String> packages) and getRootCauseStackTrace(List<String> packages) methods to take the additional String separator argument. These are public method so I'm assuming that this may not work? Is 2.0 still considered beta and can we make these types of changes? 2. Create new method signatures for getExtendedStackTrace(List<String> packages, String separator) and getRootCauseStackTrace(List<String> packages, String separator) 3. Create new options container StackTraceOptions.java for instance that contains the List<String> packages and String separator members then add new method signatures for those. This may actually require that I create a private getExtendedStackTrace(List<String> packages, String separator) and getRootCauseStackTrace(List<String> packages, String separator) to be compatible with the existing methods. #3 seems to be the best OO approach that would support new options in the future. One other question, which of the following do you prefer for the prefix (maybe separator is more consistent): %throwable{delim( | )} %throwable{sep( | )} %throwable{separator( | )} Thanks for your help! Joanne From: [email protected] Subject: Re: Feature Request from 1.x: Include option in throwable pattern converter to control stack trace separator Date: Fri, 1 Feb 2013 12:05:23 -0800 To: [email protected] I should add that since you've identified them it would be good to make sure there is a unit test for each of them. Ralph On Feb 1, 2013, at 12:04 PM, Ralph Goers wrote:Yes - this all sounds good to me. Ralph On Feb 1, 2013, at 11:27 AM, Joanne Polsky wrote:Thanks Ralph and Gary, that makes more sense. I indicated it would default to "\n" since that is what is hard coded in the source now. I can update to Constants.LINE_SEP while adding this new feature. The ThrowablePatternConverter currently doesn't parse the filters, so having delim as it's own option may be simpler in the end. I would think the following patterns should be supported: %throwable{full}{delim( | )} %throwable{delim( | )} %throwable %xThrowable{full}{filters(package1,package2)}{delim( | )} %xThrowable{full}{delim( | )} %xThrowable{delim( | )} %xThrowable Also the delim could be more than one character, for instance we use space+pipe+space as our separator. Would those patterns make sense? Joanne From: [email protected] Subject: Re: Feature Request from 1.x: Include option in throwable pattern converter to control stack trace separator Date: Fri, 1 Feb 2013 11:19:35 -0800 To: [email protected] Yes. That is available as Constants.LINE_SEP. Ralph On Feb 1, 2013, at 11:03 AM, Gary Gregory wrote:Shouldn't the default be the line.separator system property? Gary On Fri, Feb 1, 2013 at 1:54 PM, Ralph Goers <[email protected]> wrote: First, thanks for participating! The process you are following is perfectly fine. It is always great to start a discussion of a new feature on the dev list to work out any details. You can create a Jira with the feature request and then attach a patch once you have completed the work. Assigning the Jira to yourself lets us know that you plan to work on it so that is a good idea. We could either use the syntax you are proposing or %xEx{full}{filters(package1,package2),delim(|)} which would be a little shorter. In both cases I would expect specifying the filters should be optional if the delimiter is included. FWIW, this change will be a bit more extensive than just changing line 115. The extended throwable pattern converters do most of their formatting in ThrowableProxy, so it would need to be made aware of the delimiter. Ralph On Feb 1, 2013, at 9:54 AM, Joanne Polsky wrote: Hello, Some time ago, I had submitted a feature request for Log4j 1.x which I never got around to actually implementing: https://issues.apache.org/bugzilla/show_bug.cgi?id=51122 I see that this feature request would apply to the new Log4j 2.x source as well: http://svn.apache.org/viewvc/logging/log4j/log4j2/trunk/core/src/main/java/org/apache/logging/log4j/core/pattern/ThrowablePatternConverter.java?view=markup Now that I actually have some time, I'd like to make this contribution if no one has any concerns. Essentially the change is to make the appended new line configurable to be any string delimiter default being "\n": [ThrowablePatternConverter.java:115] sb.append(array[i]).append("\n"); I had originally thought that a pattern like the following might work "%throwable{full}{ | }". However, it looks like in 2.0, there are some classes which extend the ThrowablePatternConverter that assume the second option may be a list of filters: https://svn.apache.org/repos/asf/logging/log4j/log4j2/trunk/core/src/main/java/org/apache/logging/log4j/core/pattern/ExtendedThrowablePatternConverter.java https://svn.apache.org/repos/asf/logging/log4j/log4j2/trunk/core/src/main/java/org/apache/logging/log4j/core/pattern/RootThrowablePatternConverter.java I was thinking I could still implement this change in ThrowablePatternConverter where it would iterate through the options looking for the specific prefix of delim? So for instance the user would specify something like the following (not sure if I like that delim but I can't think of another identifier): %throwable{full}{delim( | )} %XThrowable{full}{filters(package1,package2)}{delim( | )} %rThrowable{full}{filters(package1,package2)}{delim( | )} This would be my first contribution to any Apache software if this feature was accepted so I'm not exactly sure how to proceed if I'm interested in making the contribution myself. I guess the first step would be to to copy the feature request from the 1.0 bugzilla to the 2.0 JIRA (https://issues.apache.org/jira/browse/LOG4J2). Is there some approval process for the feature before I start implementing the change? Or would I simply assign the JIRA to myself, make the code change I think makes sense, then submit for code review? Thanks, Joanne -- E-Mail: [email protected] | [email protected] JUnit in Action, 2nd Ed: http://bit.ly/ECvg0 Spring Batch in Action: http://bit.ly/bqpbCK Blog: http://garygregory.wordpress.com Home: http://garygregory.com/ Tweet! http://twitter.com/GaryGregory
