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