Re: [Codenarc-user] LoggerWithWrongModifiersRule supporting subclasses
Brought to you by:
chrismair
Menu
▾
▴
Re: [Codenarc-user] LoggerWithWrongModifiersRule supporting subclasses
|
From: Hamlet D. <ham...@ca...> - 2011-06-01 09:36:46
|
> I think two rules for the logger is adding too much complexity. I > still > favor my original patch. This supports the two logger instantiation There are already two different rules since version 0.12: one for logger modifiers(LoggerWithWrongModifiersRule) and one for the parameter to the logger factory (LoggerForDifferentClassRule). The problem with the patch was that it added logic about the parameter into the rule about the visibility. So there were no longer two distinct rules, but two rules that overlapped with each other. If you want logic about accepting "this.getClass()" as a valid parameter, then that logic belongs in LoggerForDifferentClassRule. If you want logic about accepting a non-static logger, then that logic belongs in LoggerWithWrongModifiersRule. If you want to allow a non-static logger that accepts "this.getClass()" as a parameter then you need to configure two rules. Considering these properties: > > LoggerWithWrongModifiersRule has 2 properties: > > boolean allowProtectedLogger = false > > boolean allowNonStaticLogger = false > > > > LoggerForDifferentClassRule has 1 property: > > boolean allowDerivedLogger = false By default, the rules do what is right and most people will never know these rules have properties. There is no complexity for most of users, yet for those that want finer control then every use case is covered. I would leave the extra properties in. The default configuration does the right thing, and every use case is covered for those that want something different... What I would not do is accept the original patch because it mixes the logic about parameter checking between two different rules. -- Hamlet D'Arcy ----- Original Message ----- > My thoughts match Chris'. > > I think two rules for the logger is adding too much complexity. I > still > favor my original patch. This supports the two logger instantiation > cases I have seen and classified as correct in recent years. > > Sure there are more cases out there (injecting via Spring, per > thread, > new Logger, etc.) but this rule should stick to the common ones. If > there are too many posibilities it will just create confusion. In > case > we missed a relevant case people will raise their hands. > > Regards, > René Scheibe > > On 06/01/2011 02:13 AM, Chris Mair wrote: > > Hamlet / René, > > > > Thanks for working on this Hamlet. I'm sorry I did not respond > > sooner. I see no value, for me anyway, in the > > LoggerWithWrongModifiersRule supporting allowProtectedLogger or > > allowNonStaticLogger properties. I will not enable those because I > > do want to find the genuine bugs where a logger is incorrectly > > defined as protected or as non-static. I guess I will just > > configure exceptions for those (relatively rare) framework > > superclasses where I define the LOG in the superclass for all > > child classes. > > > > Will you use those properties René? If they provide value, then > > fine. But if we are adding complexity that no one will (or should) > > use, then I don't want to do that. > > > > Chris > > -----Original Message----- > > From: Hamlet DArcy [mailto:ham...@ca...] > > Sent: Tuesday, May 31, 2011 4:55 AM > > To: René Scheibe > > Cc: Cod...@li... > > Subject: Re: [Codenarc-user] LoggerWithWrongModifiersRule > > supporting subclasses > > > > I think the best solution is this because it covers every use case: > > > > LoggerWithWrongModifiersRule has 2 properties: > > boolean allowProtectedLogger = false > > boolean allowNonStaticLogger = false > > > > LoggerForDifferentClassRule has 1 property: > > boolean allowDerivedLogger = false > > > > > > Then everything is covered for everyone with no overlap. > > > > > > ----- Original Message ----- > >> It's not about the "protected" (this is just required). > >> > >> The interesting part is the non-static one. Because of it, a > >> superclass uses the logger of the derived class based on > >> "getLogger(this.class)". > >> > >> With such a logger it's for example possible to write every log > >> message of a class (including its superclass) to a separate file > >> because they are using the same logger. > >> > >> Regards, > >> René Scheibe > >> > >> On 05/31/2011 09:55 AM, Hamlet DArcy wrote: > >>> Personally, I would require loggers to be static, private, and > >>> final. > >>> If someone really needed a protected logger then i would still > >>> make > >>> it static and final, just protected. > >>> > >>> So I think we need both these settings. > >>> > >>> > >>> > >>> ----- Original Message ----- > >>>> That's a great question. > >>>> > >>>> I do use that pattern in framework superclasses: > >>>> protected final LOG = Logger.getLogger(this.class) > >>>> > >>>> to provide a built-in logger for all subclasses. And so I have > >>>> to > >>>> configure exceptions for this rule for those superclasses. > >>>> > >>>> Is there any value in splitting up the "exception" configuration > >>>> into two properties, if we do allow it? > >>>> * allowNonStaticLogger > >>>> * allowProtectedLogger > >>>> > >>>> Are there use cases we can envision where we (or anyone) would > >>>> choose to allow non-static but not protected, or vice versa? > >>>> > >>>> Chris > >>>> -----Original Message----- > >>>> From: Hamlet D'Arcy [mailto:ham...@gm...] > >>>> Sent: Monday, May 30, 2011 2:50 AM > >>>> To: René Scheibe; Cod...@li... > >>>> Subject: [Codenarc-user] LoggerWithWrongModifiersRule supporting > >>>> subclasses > >>>> > >>>> Hi everyone, > >>>> > >>>> This email is about the idea about the > >>>> LoggerWithWrongModifiersRule > >>>> supporting subclasses. > >>>> > >>>> René filed an issue (and supplied a patch) describing the issue: > >>>> https://sourceforge.net/tracker/index.php?func=detail&aid=3308930&g > >>>> roup_id=2 > >>>> 50145&atid=1126575# > >>>> > >>>> Basically, the question is how subclasses should be allowed to > >>>> access a Logger in a parent class. > >>>> > >>>> Normally Loggers are private, static, and final. This patch > >>>> allows > >>>> you to accept this as valid code when you turn a certain > >>>> property > >>>> on: > >>>> > >>>> protected final LOG = Logger.getLogger(this.class) > >>>> > >>>> I'd like to discuss this just a little more before committing > >>>> the > >>>> patch. > >>>> > >>>> With this change, it is acceptable to have a non-static logger. > >>>> So > >>>> each instance of a subclass makes and creates a logger. This > >>>> does > >>>> not seem right, and I would reject it in a code review as > >>>> improper > >>>> logging. I would accept this form though: > >>>> > >>>> protected static final LOG = Logger.getLogger(MyClass.class) > >>>> > >>>> Do we really want to allow non-static Loggers? > >>>> > >>>> if so, I recommend two new properties instead of just one: > >>>> * allowNonStaticLogger > >>>> * allowProtectedLogger > >>>> > >>>> Then the user can control the behavior a little better. > >>>> > >>>> > >>>> > >>>> -- > >>>> Hamlet D'Arcy > >>>> ham...@gm... > >>>> > >>>> ------------------------------------------------------------------- > >>>> --------- > >>>> -- > >>>> vRanger cuts backup time in half-while increasing security. > >>>> With the market-leading solution for virtual backup and > >>>> recovery, > >>>> you get blazing-fast, flexible, and affordable data protection. > >>>> Download your free trial now. > >>>> http://p.sf.net/sfu/quest-d2dcopy1 > >>>> _______________________________________________ > >>>> Codenarc-user mailing list > >>>> Cod...@li... > >>>> https://lists.sourceforge.net/lists/listinfo/codenarc-user > >>>> > >>>> > >>>> ------------------------------------------------------------------- > >>>> ----------- Simplify data backup and recovery for your virtual > >>>> environment with vRanger. > >>>> Installation's a snap, and flexible recovery options mean your > >>>> data > >>>> is safe, secure and there when you need it. Data protection > >>>> magic? > >>>> Nope - It's vRanger. Get your free trial download today. > >>>> http://p.sf.net/sfu/quest-sfdev2dev > >>>> _______________________________________________ > >>>> Codenarc-user mailing list > >>>> Cod...@li... > >>>> https://lists.sourceforge.net/lists/listinfo/codenarc-user > >>>> > >> > >> > >> ---------------------------------------------------------------------- > >> -------- Simplify data backup and recovery for your virtual > >> environment with vRanger. > >> Installation's a snap, and flexible recovery options mean your > >> data is > >> safe, secure and there when you need it. Data protection magic? > >> Nope - It's vRanger. Get your free trial download today. > >> http://p.sf.net/sfu/quest-sfdev2dev > >> _______________________________________________ > >> Codenarc-user mailing list > >> Cod...@li... > >> https://lists.sourceforge.net/lists/listinfo/codenarc-user > >> > > > > ------------------------------------------------------------------------------ > > Simplify data backup and recovery for your virtual environment with > > vRanger. > > Installation's a snap, and flexible recovery options mean your data > > is safe, secure and there when you need it. Data protection magic? > > Nope - It's vRanger. Get your free trial download today. > > http://p.sf.net/sfu/quest-sfdev2dev > > _______________________________________________ > > Codenarc-user mailing list > > Cod...@li... > > https://lists.sourceforge.net/lists/listinfo/codenarc-user > > > > |
