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
>>>>
>>>
>>
>
