I would be OK with getting rid of the ordinal. It makes it less enum-like, but I agree that the ordinal really has little purpose now. The intLevel is more important.
Here may be the best approach I can think of for calculating the StandardLevel-equivalent on instantiation: public static Level OFF = new Level("OFF", 0, StandardLevel.OFF) {}; ... public static Level ALL = new Level("ALL", Integer.MAX_VALUE, StandardLevel.ALL); ... private Level(String name, int intLevel, StandardLevel standardLevel) { // this is the only c-tor standard levels use // same logic as current constructor this.standardLevel = standardLevel; } protected Level(String name, int intLevel) { // this is the only c-tor custom levels use this(name, intLevel, Level.calculateStandardLevel(intLevel)); } public enum StandardLevel { OFF, FATAL, ERROR, WARN, INFO, DEBUG, TRACE, ALL } Thoughts? N On Jan 26, 2014, at 4:02 PM, Ralph Goers wrote: > I do have one other comment. You mention that the ordinal value isn’t > guaranteed because the levels might be instantiated in a different order each > time. An alternative wold be to just get rid of the ordinal. It isn’t used > anywhere by anything and when custom values are added they will be added > after the standard levels, which is correct but might not be what you would > expect. Eliminating that would allow the static initialization to stay as it > is and get rid of the need for synchronization in the constructor. > > Ralph > > On Jan 26, 2014, at 12:41 PM, Nick Williams <nicho...@nicholaswilliams.net> > wrote: > >> Some (ok, a lot of) feedback: >> >> - `private static ConcurrentMap<String, Level> levels` should be final. >> >> - `private static Object constructorLock` should be final. In fact, code >> inspection flags this as a warning since code synchronizes on it. >> >> - The standard Level constants should be instantiated in a static >> initializer like in my original code. Otherwise the order they are >> instantiated in is unpredictable, and DEBUG (for example) may even have a >> different ordinal each time the JVM starts. >> >> - Level isn't abstract. However, you use `new Level("xxx", n) {}` (brackets) >> for Level.OFF, ExtendedLevels.NOTE, and ExtendedLevels.DETAIL, but you use >> `new Level("xxx", n)` (no brackets) for other levels. IMO, Level should be >> abstract (so that there's always exactly one instance of every class that >> extends Level, just like a real enum), and the levels should all use {} to >> construct. However, if we don't make Level abstract, then we should remove >> {} from OFF, NOTE, and DETAIL because it's unnecessary. >> >> - The way StdLevel appears in the middle of Level separates its static >> fields from its instance fields, constructor, and methods. It makes it >> difficult to read. I have to scroll pass StdLevel to see the rest of Level. >> Can we please move StdLevel to the bottom of Level? >> >> - This is more personal preference, but I don't like abbreviating things in >> class names. What is Std? We may know, but does everyone? To someone >> unfamiliar with English, Standard is easy to translate, but they have to >> first figure out that Std is short for Std. Can be please un-abbreviate this >> to StandardLevel? >> >> - I disagree with this approach: >> >> if (levels.containsKey(name)) { >> Level level = levels.get(name); >> if (level.intLevel() != intLevel) { >> throw new IllegalArgumentException("Level " + name + " has >> already been defined."); >> } >> ordinal = level.ordinal; >> } >> >> This allows multiple Levels with the same name/intLevel to be instantiated, >> which prevents equality testing (level == Level.OFF) from being 100% >> deterministic. It should really be this: >> >> if (levels.containsKey(name)) { >> throw new IllegalArgumentException("Level " + name + " has >> already been defined."); >> } >> >> - I think we should make all of the methods of Level final. Custom levels >> shouldn't be able to change their behavior, IMO. Alternatively, if we don't >> make Level abstract, we should make it final. >> >> - Level has no JavaDoc. The Level constants in Level have no JavaDoc. That >> needs to be fixed. >> >> - I'm still not convinced StdLevel/StandardLevel is necessary. It seems like >> an anti-pattern to me. From what I can tell (please let me know if I'm >> missing something), StdLevel's entire purpose is to allow us to still switch >> on the standard levels. I think that's a bad reason to create an enum whose >> constants mirror Level. The primary problem with the StdLevel is that the >> conversion from a Level to a StdLevel in many cases happens __on every >> logging event__. That's going to be hugely inefficient. Really, when a >> custom Level is created, it should be created with an equivalent standard >> Level (or StdLevel). There are several ways to accomplish this that are open >> to discussion, but I don't think the current StdLevel.getStdLevel is the >> right approach. One alternative: >> >> private final Level standardLevel; >> >> protected Level(String name, int level, Level mapToOtherFrameworksAs) { >> this(name, level); >> this.standardLevel = mapToOtherFrameworksAs; >> } >> >> private Level(String name, int level) { >> // same as now >> this.standardLevel = this; >> } >> >> public final Level getStandardLevel() { >> return this.standardLevel; >> } >> >> This does, admittedly, have some problems. Another alternative that I could >> also be happy with that keeps the StandardLevel enum: >> >> private final StandardLevel standardLevel; >> >> protected Level(String name, int level, StandardLevel >> mapToOtherFrameworksAs) { >> this(name, level); >> this.standardLevel = mapToOtherFrameworksAs; >> } >> >> private Level(String name, int level) { >> // same as now >> this.standardLevel = this; >> } >> >> public final StandardLevel getStandardLevel() { >> return this.standardLevel; >> } >> >> Nick >> >> On Jan 26, 2014, at 1:38 PM, Ralph Goers wrote: >> >>> I’ve committed the changes. Take a look at ExtendedLevels.java, >>> ExtendedLevelTest.java and log4j-customLevel.xml in the log4j-core test >>> directories to see how it works. >>> >>> Ralph >>> >>> On Jan 26, 2014, at 1:19 AM, Remko Popma <remko.po...@gmail.com> wrote: >>> >>>> I'm very curious! Can't wait to see it. Go for it! >>>> >>>> On Sunday, January 26, 2014, Ralph Goers <ralph.go...@dslextreme.com> >>>> wrote: >>>> I have completed the work on custom levels. It uses a variation of Nick’s >>>> “extensible enum” class. The major difference with what he proposed is >>>> that the custom enums must be declared in a class annotated with >>>> @Plugin(name=“xxxx” category=“Level”) for them to be usable during >>>> configuration. >>>> >>>> Are their any objections to me checking this in? I’ll be doing the commit >>>> at around noon Pacific Daylight Time if I don’t hear any. >>>> >>>> Ralph >>>> >>>> >>>> >>>> On Jan 25, 2014, at 7:08 AM, Ralph Goers <ralph.go...@dslextreme.com> >>>> wrote: >>>> >>>>> I am working on the implementation of custom levels now. I should have >>>>> it done today. >>>>> >>>>> Ralph >>>>> >>>>> On Jan 24, 2014, at 7:07 PM, Remko Popma <remko.po...@gmail.com> wrote: >>>>> >>>>>> What is the best way to make progress on the custom levels >>>>>> implementation? >>>>>> >>>>>> Do we re-open LOG4J-41 or start a fresh Jira ticket? For implementation >>>>>> ideas, do we attach files to Jira, or create a branch? >>>>>> >>>>>> Remko >>>>>> >>>>>> On Saturday, January 25, 2014, Gary Gregory <garydgreg...@gmail.com> >>>>>> wrote: >>>>>> On Fri, Jan 24, 2014 at 11:48 AM, Remko Popma <remko.po...@gmail.com> >>>>>> wrote: >>>>>> Gary, >>>>>> >>>>>> The hard-coded levels were proposed because it seemed that the >>>>>> extensible enum idea raised by Nick was not going to be accepted. >>>>>> My original position was that Markers could fulfill the requirement but >>>>>> Nick and yourself made it clear that this was not satisfactory. >>>>>> >>>>>> With extensible enums and markers off the table it seemed that the >>>>>> hard-coded levels was the only alternative, and discussion ensued about >>>>>> what these levels should be called and what strength they should have. >>>>>> >>>>>> During this discussion, several people, including me, repeatedly >>>>>> expressed strong reservations about adding pre-defined levels, but by >>>>>> this time I think people were thinking there was no alternative. >>>>>> >>>>>> It looked like we were getting stuck, with half the group moving in one >>>>>> direction ("add pre-defined levels!") and the other half wanting to move >>>>>> in another direction ("don't add pre-defined levels!"). I asked that we >>>>>> re-reviewed our assumptions and try to reach a solution that would >>>>>> satisfy all users. >>>>>> >>>>>> We then decided to explore the option of using extensible enums again. >>>>>> This is still ongoing, but I haven't seen anyone arguing against this >>>>>> idea since we started this thread. >>>>>> >>>>>> Hard-coded levels and the extensible enum are different solutions to the >>>>>> same problem. >>>>>> >>>>>> Hello All: >>>>>> >>>>>> Absolutely not. See my DEFCON example. >>>>>> Talking about an "extensible enum" is mixing design and implementation, >>>>>> we are talking about 'custom' and/or 'extensible' levels. >>>>>> Custom/Extensible levels can be designed to serve one or all of: >>>>>> >>>>>> - Allow inserting custom levels between built-in levels. >>>>>> - Allow for domain specific levels outside of the concept of built-in >>>>>> levels, the DEFCON example. >>>>>> - Should the custom levels themselves be extensible? >>>>>> >>>>>> Gary >>>>>> >>>>>> The extensible enum solution satisfies all of us who are opposed to >>>>>> adding pre-defined levels, while also satisfying the original >>>>>> requirement raised by Nick and yourself. Frankly I don't understand why >>>>>> you would still want the pre-defined levels. >>>>>> >>>>>> Remko >>>>>> >>>>>> >>>>>> >>>>>> On Sat, Jan 25, 2014 at 12:53 AM, Gary Gregory <garydgreg...@gmail.com> >>>>>> wrote: >>>>>> On Thu, Jan 23, 2014 at 10:45 PM, Remko Popma <remko.po...@gmail.com> >>>>>> wrote: >>>>>> Gary, >>>>>> >>>>>> I think that's a very cool idea! >>>>>> Much more flexible, powerful and elegant than pre-defined levels could >>>>>> ever be. >>>>>> >>>>>> As I wrote: "I am discussing custom levels here with the understanding >>>>>> that this is a separate topic from what the built-in levels are." >>>>>> >>>>>> I'm not sure why you want to make the features mutually exclusive. >>>>>> (Some) others agree that these are different features. >>>>>> >>>>>> I see two topics: >>>>>> >>>>>> - What are the default levels for a 21st century logging framework. Do >>>>>> we simply blindly copy Log4j 1? Or do we look at frameworks from >>>>>> different languages and platforms for inspiration? >>>>>> - How (not if, I think we all agree) should we allow for custom levels. >>>>>> >>>>>> Gary >>>>>> >>>>>> It definitely makes sense to design the extensible enum with this >>>>>> potential usage in mind. >>>>>> >>>>>> Remko >>>>>> >>>>>> >>>>>> On Friday, January 24, 2014, Gary Gregory <garydgreg...@gmail.com> wrote: >>>>>> I am discussing custom levels here with the understanding that this is a >>>>>> separate topic from what the built-in levels are. Here is how I >>>>>> convinced myself that custom levels are a “good thing”. >>>>>> >>>>>> No matter which built-in levels exits, I may want custom levels. For >>>>>> example, I want my app to use the following levels DEFCON1, DEFCON2, >>>>>> DEFCON3, DEFCON4, and DEFCON5. This might be for one part of my app or a >>>>>> whole subsystem, no matter, I want to use the built-in levels in >>>>>> addition to the DEFCON levels. It is worth mentioning that if I want >>>>>> that feature only as a user, I can “skin” levels in a layout and assign >>>>>> any label to the built-in levels. If I am also a developer, I want to >>>>>> use DEFCON levels in the source code. >>>>>> >>>>>> >>>>>> >>>>>> At first, my code might look like: >>>>>> >>>>>> >>>>>> >>>>>> logger.log(DefconLevels.DEFCON5, “All is quiet”); >>>>>> >>>>>> >>>>>> >>>>>> Let’s put aside for now the type of DefconLevels.DEFCON* objects. I am a >>>>>> user, and I care about my call sites. >>>>>> >>>>>> >>>>>> >>>>>> What I really want of course is to write: >>>>>> >>>>>> >>>>>> >>>>>> defconLogger.defcon5(“All is quiet”) >>>>>> >>>>>> >>>>>> >>>>>> Therefore, I argue that for any “serious” use of a custom level, I will >>>>>> wrap a Logger in a custom logger class providing call-site friendly >>>>>> methods like defcon5(String). >>>>>> >>>>>> >>>>>> >>>>>> So now, as a developer, all I care about is DefConLogger. It might wrap >>>>>> (or subclass) the Log4J Logger, who knows. The implementation of >>>>>> DefConLogger is not important to the developer (all I care is that the >>>>>> class has ‘defconN’ method) but it is important to the configuration >>>>>> author. This tells me that as a developer I do not care how DefConLogger >>>>>> is implemented, with custom levels, markers, or elves. However, as >>>>>> configuration author, I also want to use DEFCON level just like the >>>>>> built-in levels. >>>>>> >>>>>> >>>>>> >>>>>> The configuration code co >>>>>> >>>>>> >>>>>> >>>>>> >>>>>> -- >>>>>> E-Mail: garydgreg...@gmail.com | ggreg...@apache.org >>>>>> Java Persistence with Hibernate, Second Edition >>>>>> JUnit in Action, Second Edition >>>>>> Spring Batch in Action >>>>>> Blog: http://garygregory.wordpress.com >>>>>> Home: http://garygregory.com/ >>>>>> Tweet! http://twitter.com/GaryGregory >>>>> >>>> >>> >> >> >> --------------------------------------------------------------------- >> To unsubscribe, e-mail: log4j-dev-unsubscr...@logging.apache.org >> For additional commands, e-mail: log4j-dev-h...@logging.apache.org >> > > > --------------------------------------------------------------------- > To unsubscribe, e-mail: log4j-dev-unsubscr...@logging.apache.org > For additional commands, e-mail: log4j-dev-h...@logging.apache.org > --------------------------------------------------------------------- To unsubscribe, e-mail: log4j-dev-unsubscr...@logging.apache.org For additional commands, e-mail: log4j-dev-h...@logging.apache.org