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 >
