The updated PR looks good to me. Remko.
(Shameless plug) Every java main() method deserves http://picocli.info > On Nov 4, 2018, at 1:11, Carter Kozak <[email protected]> wrote: > > 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 >>>
