No. I think the guidelines are pretty clear. It is just that making changes just because you think something would look better just creates more work for the rest of us as we review unnecessary changes. And in cases like this they might have a detrimental impact.
Even then the code wouldn’t be RTC. All you had to do was say “hey, I noticed that we are checking for null in various places before calling a method. Wouldn’t it be better to do the null check as the first line in the method?” Then we could have discussed that and decided whether or not that was something we should learn from and make a best practice or whether there is a reason it should be the way it is. The use of the final keyword is a great example. We had a good discussion and, I believe, came to a consensus. The only issue there was that it happened after you started making the changes. I have no problem with you making changes that fix bugs or provide new features that are tracked by Jira issues without discussion, unless the solution requires it. I have always had problems with commits that are simply to use “better” variable names. I am perfectly fine with variable names that are short and concise or are abbreviations. You consistently change them because you think they are “better”. I don’t change them back simply because when I have time to work on Log4j I prefer to work on real problems.’ Ralph > On Dec 2, 2014, at 8:33 PM, Gary Gregory <[email protected]> wrote: > > On Tue, Dec 2, 2014 at 9:49 PM, Ralph Goers <[email protected] > <mailto:[email protected]>> wrote: > 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. > > Wow, well, that seems like a pretty small change to be worth a ML discussion. > I'll have to think twice before touching code if we are going to do RTC for > stuff like this. I thought we developed in CTR mode. Are you saying we should > do RTC? > > Gary > > > Ralph > > >> On Dec 2, 2014, at 6:54 PM, Gary Gregory <[email protected] >> <mailto:[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> > > > > -- > 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>
