Please see http://logging.apache.org/log4j/2.x/guidelines.html#teamwork <http://logging.apache.org/log4j/2.x/guidelines.html#teamwork> item 4.
“Fixing” things that only you have identified as a problem should have been preceded by a discussion. Ralph > On Dec 2, 2014, at 6:54 PM, Gary Gregory <[email protected]> wrote: > > On Tue, Dec 2, 2014 at 6:37 PM, Ralph Goers <[email protected] > <mailto:[email protected]>> wrote: > Why did you make the change? > > Because I do not like code duplication and we had null checks before calling > a method. It looks less cluttered now IMO. > > Was something broken? > > Not for the normal definition of "broken". But we don't have to only make > changes to "fix" something... > > Gary > > > Ralph > >> On Dec 2, 2014, at 3:08 PM, Gary Gregory <[email protected] >> <mailto:[email protected]>> wrote: >> >> Yes: This is a change that refactors guard clauses, no behavioral change. >> Tests pass. >> No: I do not know if the specific case you mention is covered by the unit >> tests or the code for that matter. The tests do pass though. >> Thoughts? >> >> Gary >> >> On Tue, Dec 2, 2014 at 5:01 PM, Remko Popma <[email protected] >> <mailto:[email protected]>> wrote: >> Are you sure this is correct? >> What about deserialized ThrowableProxies where the cause is null but the >> causeProxy is non-null? >> >> Sent from my iPhone >> >> > On 2014/12/03, at 3:10, [email protected] <mailto:[email protected]> >> > wrote: >> > >> > Repository: logging-log4j2 >> > Updated Branches: >> > refs/heads/master ab58a6d69 -> df71c934d >> > >> > >> > Let the formatCause method have a null guard clause instead of each call >> > site guarding the call. >> > >> > Project: http://git-wip-us.apache.org/repos/asf/logging-log4j2/repo >> > <http://git-wip-us.apache.org/repos/asf/logging-log4j2/repo> >> > Commit: >> > http://git-wip-us.apache.org/repos/asf/logging-log4j2/commit/df71c934 >> > <http://git-wip-us.apache.org/repos/asf/logging-log4j2/commit/df71c934> >> > Tree: http://git-wip-us.apache.org/repos/asf/logging-log4j2/tree/df71c934 >> > <http://git-wip-us.apache.org/repos/asf/logging-log4j2/tree/df71c934> >> > Diff: http://git-wip-us.apache.org/repos/asf/logging-log4j2/diff/df71c934 >> > <http://git-wip-us.apache.org/repos/asf/logging-log4j2/diff/df71c934> >> > >> > Branch: refs/heads/master >> > Commit: df71c934d59570bbee10d8eb413d92eaa443bec4 >> > Parents: ab58a6d >> > Author: Gary Gregory <[email protected] >> > <mailto:[email protected]>> >> > Authored: Tue Dec 2 13:10:10 2014 -0500 >> > Committer: Gary Gregory <[email protected] >> > <mailto:[email protected]>> >> > Committed: Tue Dec 2 13:10:10 2014 -0500 >> > >> > ---------------------------------------------------------------------- >> > .../apache/logging/log4j/core/impl/ThrowableProxy.java | 11 +++++------ >> > 1 file changed, 5 insertions(+), 6 deletions(-) >> > ---------------------------------------------------------------------- >> > >> > >> > http://git-wip-us.apache.org/repos/asf/logging-log4j2/blob/df71c934/log4j-core/src/main/java/org/apache/logging/log4j/core/impl/ThrowableProxy.java >> > >> > <http://git-wip-us.apache.org/repos/asf/logging-log4j2/blob/df71c934/log4j-core/src/main/java/org/apache/logging/log4j/core/impl/ThrowableProxy.java> >> > ---------------------------------------------------------------------- >> > diff --git >> > a/log4j-core/src/main/java/org/apache/logging/log4j/core/impl/ThrowableProxy.java >> > >> > b/log4j-core/src/main/java/org/apache/logging/log4j/core/impl/ThrowableProxy.java >> > index 3aadf6c..23065ab 100644 >> > --- >> > a/log4j-core/src/main/java/org/apache/logging/log4j/core/impl/ThrowableProxy.java >> > +++ >> > b/log4j-core/src/main/java/org/apache/logging/log4j/core/impl/ThrowableProxy.java >> > @@ -183,12 +183,13 @@ public class ThrowableProxy implements Serializable { >> > >> > @SuppressWarnings("ThrowableResultOfMethodCallIgnored") >> > private void formatCause(final StringBuilder sb, final ThrowableProxy >> > cause, final List<String> ignorePackages) { >> > + if (cause == null) { >> > + return; >> > + } >> > sb.append("Caused by: ").append(cause).append(EOL); >> > this.formatElements(sb, cause.commonElementCount, >> > cause.getThrowable().getStackTrace(), >> > cause.extendedStackTrace, ignorePackages); >> > - if (cause.getCauseProxy() != null) { >> > - this.formatCause(sb, cause.causeProxy, ignorePackages); >> > - } >> > + this.formatCause(sb, cause.causeProxy, ignorePackages); >> > } >> > >> > private void formatElements(final StringBuilder sb, final int >> > commonCount, final StackTraceElement[] causedTrace, >> > @@ -342,9 +343,7 @@ public class ThrowableProxy implements Serializable { >> > } >> > sb.append('\n'); >> > this.formatElements(sb, 0, this.throwable.getStackTrace(), >> > this.extendedStackTrace, ignorePackages); >> > - if (this.causeProxy != null) { >> > - this.formatCause(sb, this.causeProxy, ignorePackages); >> > - } >> > + this.formatCause(sb, this.causeProxy, ignorePackages); >> > return sb.toString(); >> > } >> > >> > >> >> --------------------------------------------------------------------- >> To unsubscribe, e-mail: [email protected] >> <mailto:[email protected]> >> For additional commands, e-mail: [email protected] >> <mailto:[email protected]> >> >> >> >> >> -- >> E-Mail: [email protected] <mailto:[email protected]> | >> [email protected] <mailto:[email protected]> >> Java Persistence with Hibernate, Second Edition >> <http://www.manning.com/bauer3/> >> JUnit in Action, Second Edition <http://www.manning.com/tahchiev/> >> Spring Batch in Action <http://www.manning.com/templier/> >> Blog: http://garygregory.wordpress.com <http://garygregory.wordpress.com/> >> Home: http://garygregory.com/ <http://garygregory.com/> >> Tweet! http://twitter.com/GaryGregory <http://twitter.com/GaryGregory> > > > > -- > E-Mail: [email protected] <mailto:[email protected]> | > [email protected] <mailto:[email protected]> > Java Persistence with Hibernate, Second Edition > <http://www.manning.com/bauer3/> > JUnit in Action, Second Edition <http://www.manning.com/tahchiev/> > Spring Batch in Action <http://www.manning.com/templier/> > Blog: http://garygregory.wordpress.com <http://garygregory.wordpress.com/> > Home: http://garygregory.com/ <http://garygregory.com/> > Tweet! http://twitter.com/GaryGregory <http://twitter.com/GaryGregory>
