There are two conversations. The first is on ignore vs. suppress. The
second is on javadocing cleary what the chosen term means.
On Jul 20, 2013 5:42 PM, "Ralph Goers" <[email protected]> wrote:
> Well - I saw "hold everything" and then that was followed by "we need
> proper documentation". So I got the impression this was going to be a
> documentation change, not a code change (except, I suppose to fix the
> handleExceptions variable). But you made the change and it isn't worth the
> trouble to change it back (or to some other variable name next week when
> someone complains they don't like ignoreExceptions).
>
> Ralph
>
> On Jul 20, 2013, at 3:33 PM, Nick Williams wrote:
>
> Well, first, my main intent for this whole thing was consistency. We have
> consistency now.
>
> In the message you reference, I said that I could see how ignoreExceptions
> could also be somewhat confusing (due to the dictionary meaning of ignore),
> but also said that no matter what we did it would need to be documented
> clearly (which it is now, by the way). I didn't say I liked suppress better
> (I don't). At least two of the other guys agreed that ignore was still
> better than suppress due to the Java feature of suppressed exceptions, so
> having heard nothing else for a couple of days I proceeded.
>
> Nick
>
> On Jul 20, 2013, at 5:27 PM, Ralph Goers wrote:
>
> I'm confused. With your message on the 18th I thought you had changed
> your mind and agreed that "suppressExceptions" was a better choice than
> "ignoreExceptions" since it more accurately describes what is being done.
>
> Ralph
>
> On Jul 20, 2013, at 2:58 PM, Nick Williams wrote:
>
> The conversion from
> "handleExceptions"/"suppressExceptions"/"isExceptionSuppressed" to
> "ignoreExceptions" has been completed.
>
> I still have some work to do to make sure these are being used/abided by
> consistently and to make sure that all appenders and managers really do let
> exceptions propagate.
>
> Nick
>
> On Jul 20, 2013, at 3:57 PM, Ralph Goers wrote:
>
> Yeah - that seems to work.
>
> Ralph
>
> On Jul 20, 2013, at 1:35 PM, Nick Williams wrote:
>
> You left out the exclamation point.
>
> If value of "junk" is specified and the defaultValue is true then
> (!"false".equalsIgnoreCase("junk") && true) returns true.
>
> However, it should actually be (defaultValue &&
> !"false".equalsIgnoreCase(s)) so that it short-circuits it defaultValue is
> false.
>
> Nick
>
> On Jul 20, 2013, at 3:30 PM, Ralph Goers wrote:
>
> Why does this not look right to me? If a value of "junk" is specified and
> the defaultValue is true then ("false".equalsIgnoreCase("junk") && true)
> return false, which is incorrect. It should just return the default value.
>
> Ralph
>
> On Jul 20, 2013, at 1:18 PM, Nick Williams wrote:
>
> Correction below:
>
> On Jul 20, 2013, at 2:38 PM, Nick Williams wrote:
>
> Finally got back to working on this. Noticed two things:
>
> 1) On some appenders, ignoreExceptions/suppressExceptions defaults to
> true. On other ones it defaults to false. We should be consistent in this,
> and IMO it should default to true. Does anyone have any objection to that?
>
> 2) o.a.l.l.core.helpers.Booleans.parseBoolean may or may not be working as
> expected, so I wanted to check with y'all. Here's the code:
>
> return Strings.isEmpty(s) ? defaultValue : Boolean.parseBoolean(s);
>
> Boolean.parseBoolean() doesn't have a way to specify a default value. It
> automatically defaults to false if the value passed in is anything other
> than "true." So, if someone passes in a non-empty value other than "true"
> or "false", it will always return false even if defaultValue is true. I
> propose changing it to this:
>
> return "true".equalsIgnoreCase(s) || defaultValue;
>
>
> return "true".equalsIgnoreCase(s) || (!"false".equalsIgnoreCase(s) &&
> defaultValue);
>
>
> Does anyone have any problem with that?
>
> Nick
>
> On Jul 18, 2013, at 8:12 PM, Gary Gregory wrote:
>
> On Jul 18, 2013, at 20:28, Paul Benedict <[email protected]> wrote:
>
> "Proper documentation" is the key phrase. :-)
>
>
> Yes! :)
>
> Gary
>
>
>
> On Thu, Jul 18, 2013 at 7:25 PM, Nick Williams <
> [email protected]> wrote:
>
>> Okay. Hold everything! Lol...
>>
>> I started working on this change and then realized something. "Suppress"
>> means "to forcibly put an end to," "restrain," or "prevent the development,
>> action, or expression of." However, "ignore" means "refuse to take notice
>> of or acknowledge; disregard intentionally" or "fail to consider (something
>> significant)."
>>
>> Though not guaranteed, I can see how a user would mistake the word
>> "ignore" to mean that exceptions aren't even logged, while "suppress" would
>> be more obvious to mean they aren't thrown (but also confusing with Java
>> suppressed exceptions as others pointed out). I don't think it's a big deal
>> because no matter what we do we're going to have to properly document this
>> field. But I just wanted to make sure everyone had thought about this
>> before we proceeded.
>>
>> Nick
>>
>> On Jul 18, 2013, at 6:58 PM, Nick Williams wrote:
>>
>> > Okay. I'll proceed with the change.
>> >
>> > Nick
>> >
>> > On Jul 18, 2013, at 2:08 PM, Ralph Goers wrote:
>> >
>> >> First, I appreciate having the discussion before a code change like
>> this is made. If that had been done I probably would have vetoed it. But
>> this is the ASF where everybody gets a single voice and a single vote.
>> Although I dislike this change and the effect it will have on my users I
>> won't veto it if that is what the majority wants.
>> >>
>> >> Ralph
>> >>
>> >> On Jul 18, 2013, at 11:06 AM, Nick Williams wrote:
>> >>
>> >>> Hmmm. So it sounds like we're at an impasse. Everyone except Ralph
>> seems to agree to renaming it ignoreExceptions, bug Ralph said the below in
>> opposition. Ralph, can you clarify a little? Are you objecting to just
>> renaming the XML/JSON attribute (which, really, is the only thing that
>> would break your teams' configurations)? Or are you objecting to
>> ignoreExceptions completely? I can't tell whether you're okay with the
>> isExceptionSuppressed method being renamed.
>> >>>
>> >>> I would just throw it out there that, while I understand that it
>> would be inconvenient to have to modify all your configurations, this IS
>> beta software. It is not GA yet (hopefully next month or so). Anyone who
>> uses beta software does so with the understanding that there could be
>> breaking changes before GA. If you didn't want to take that risk, I
>> respectively submit that you shouldn't have used beta software in
>> production. I'm not saying, "You're wrong." I'm just questioning the
>> motives behind your opposition. In short:
>> >>>
>> >>> If we all agree that ignoreExceptions is a better name that
>> suppressExceptions (inconvenience aside), now is the time to change it. We
>> can't after GA.
>> >>>
>> >>> Now I'm not sure how to proceed. :-P
>> >>>
>> >>> Nick
>> >>>
>> >>> On Jul 18, 2013, at 1:02 AM, Ralph Goers wrote:
>> >>>
>> >>>> I do not have a problem with renaming handleExceptions to
>> exceptionSuppressed. I do have a problem with renaming supressExceptions
>> to ignoreExceptions, primarily because I have a bunch of teams using Log4j
>> 2 in production and they would have to modify their configurations when
>> they upgrade. Furthermore, I can't in my wildest dreams imagine anyone
>> getting confused over this parameter and suppressed Throwables.
>> >>>>
>> >>>> FWIW - handleExceptions means that the Appender "handles" the
>> exception (i.e. it is suppressed). I don't recall why the variables don't
>> match - I think I might have originally exposed "handleExceptions" and
>> found that to be ambiguous and renamed the config param but not the
>> internal variable.
>> >>>>
>> >>>> Ralph
>> >>>>
>> >>>> On Jul 17, 2013, at 2:42 PM, Nick Williams wrote:
>> >>>>
>> >>>>> Appender specifies a method, isExceptionSuppressed(), which
>> indicates whether exceptions thrown while appending events should be
>> suppressed (logged instead of re-thrown).
>> >>>>>
>> >>>>> AbstractAppender implements this method with a private
>> handleExceptions field and a handleExceptions constructor argument.
>> isExceptionSuppressed() returns handleExceptions (so, supposedly, "handle
>> exceptions" means "take care of exceptions instead of the user having to
>> take care of exceptions").
>> >>>>>
>> >>>>> Everybody that extends AbstractAppender uses the same
>> handleExceptions constructor argument. They all define a suppressExceptions
>> XML attribute that is assigned to the handleExceptions constructor argument
>> in the static plugin factory method.
>> >>>>>
>> >>>>> This is all very confusing to me. I just realize that I have
>> misunderstood "handleExceptions" this whole time in the database appenders
>> and have assumed it was the opposite of isExceptionSuppressed() /
>> suppressExceptions (and, thus, have written incorrect code).
>> >>>>>
>> >>>>> Does anyone have a problem with me renaming handleExceptions to
>> exceptionSuppressed (to match the JavaBean isExceptionSuppressed method) to
>> make this less confusing?
>> >>>>>
>> >>>>> Nick
>> >>>>>
>> ---------------------------------------------------------------------
>> >>>>> To unsubscribe, e-mail: [email protected]
>> >>>>> For additional commands, e-mail: [email protected]
>> >>>>>
>> >>>>
>> >>>>
>> >>>> ---------------------------------------------------------------------
>> >>>> To unsubscribe, e-mail: [email protected]
>> >>>> For additional commands, e-mail: [email protected]
>> >>>>
>> >>>
>> >>>
>> >>> ---------------------------------------------------------------------
>> >>> To unsubscribe, e-mail: [email protected]
>> >>> For additional commands, e-mail: [email protected]
>> >>>
>> >>
>> >>
>> >> ---------------------------------------------------------------------
>> >> To unsubscribe, e-mail: [email protected]
>> >> For additional commands, e-mail: [email protected]
>> >>
>> >
>>
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: [email protected]
>> For additional commands, e-mail: [email protected]
>>
>>
>
>
> --
> Cheers,
> Paul
>
>
>
>
>
>
>
>
>
>
>
