The PR looks good to me as well.
As a future improvement we may want to fall back on a failed lookup on Level.getName() to Level.getStandardLevel().getName() for the closest matching standard level style. On Sat, Nov 3, 2018 at 11:47 AM Gary Gregory <[email protected]> wrote: > > Marco: Thank you for updating your PR. > Remko: The PR has been changed to use String keys instead of Levels. LGTM. > WDYT? > > Gary > > On Thu, Oct 25, 2018 at 12:57 AM Marco Herrn <[email protected]> wrote: > > > Hello, > > > > I expected that the second approach would be desired. > > > > I have changed the pull requests accordingly. Can you please recheck? > > > > Best regards > > Marco > > > > > > On Thu, Oct 25, 2018 at 07:29:28AM +0900, Remko Popma wrote: > > > Creating custom Levels in HighLightConverter feels wrong to me. > > > > > > It’s possible to register custom log levels programmatically as well as > > in configuration ( > > https://logging.apache.org/log4j/2.x/manual/customloglevels.html). > > > > > > If the user has not defined the log levels by the time the > > HighLightConverter is used, the user made a mistake. > > > > > > The existing code handles this mistake in a strict manner, and logs an > > error to the console. I think that’s good, it’s the best we can do to > > notify the user of their mistake. > > > (Please revert the error logging. ) > > > > > > Now, how to improve things to handle this situation gracefully? > > > What about changing the map of styles in HighLightConverter to use the > > level _name_ (String) as key instead of Level objects? > > > > > > That would allow us to register the style even though the custom Level > > has not been defined yet. If this Level is defined and used later, the > > name-based lookup will find the associated style. > > > > > > Remko. > > > > > > (Shameless plug) Every java main() method deserves http://picocli.info > > > > > > > On Oct 24, 2018, at 21:31, Marco Herrn <[email protected]> wrote: > > > > > > > > Hello, > > > > > > > > I have prepared a pull request for LOG4J2-2405. > > > > There is still one decision to be made how to handle custom log levels. > > > > I added my comments to the pull request, but duplicate it here to have > > a > > > > broader audience for discussing it: > > > > > > > > To allow styles for unknown log levels the HighlightConverter creates > > these new > > > > log levels itself with an int value of Integer.MAX_VALUE. The problem > > is, when > > > > the actual application code tries to register this log level itself, > > the given > > > > int value will be ignored and Integer.MAX_VALUE remains. This is due > > to the > > > > implementation of Level.forName(String, int), which specifies that the > > intValue > > > > is ignored when called a second time with the same level name. (This > > is > > > > actually not a problem when the unknown log level is registered before > > parsing > > > > the log4j2 config file, but we can not enforce this.) > > > > > > > > I can think of two solutions to this problem: > > > > > > > > 1. Rewrite Level.forName(String, int) to always update the intValue > > with the given > > > > one. > > > > 2. Change the HighlightConverter to store the mapping of levels to > > styles > > > > not as Map<Level, String>, but instead as Map<String, String> where > > the key is > > > > the levels name. > > > > > > > > I see option 1 as the "more correct" one, since I think when > > > > calling Level.forName(String, intValue) the user would expect that the > > last > > > > definition wins, not the first one (or at least an Exception should be > > thrown > > > > and there needs to be a ways to override the existing definition of a > > log > > > > level). However it is also a more intrusive change as it changes one > > of the > > > > base classes of the API and changes the behaviour of a public method. > > > > > > > > Option 2 is less intrusive as it only changes some internals of > > > > HighlightConverter. However I still think that the current behaviour of > > > > Level.forName(String, intValue) is incorrect and should actually be > > changed. > > > > > > > > ---- > > > > > > > > Please share your idea about this. > > > > > > > > JIRA-Ticket: https://issues.apache.org/jira/browse/LOG4J2-2405 > > > > Pull-Request: https://github.com/apache/logging-log4j2/pull/225 > > > > > > > > Best regards > > > > Marco > >
