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