Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a warning in case of incorrect syntax of highlighting style. #1637

Merged
merged 2 commits into from
Oct 5, 2023

Conversation

awadammar
Copy link
Contributor

@awadammar awadammar commented Jul 30, 2023

Add a warning in case of incorrect syntax of highlighting style (without =).

This PR is related to #issue1545, it meant to ensure that the user is providing a correct syntax for highlighting style.

e.g. %highlight{%5level}{LOGBACK} should be %highlight{%5level}{STYLE=LOGBACK}. For more information see the documentation here.

@awadammar
Copy link
Contributor Author

@ppkarwasz I wonder If there's a way to check on a specific logged message in one of the unit tests?

Copy link
Member

@garydgregory garydgregory left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better message IMO:
Syntax error, missing '=': Expected "{KEY1=VALUE, KEY2=VALUE, ...}"

@awadammar
Copy link
Contributor Author

Noted, but I have a question regarding the following code block:

final String string = options[1]
              .replaceAll(PatternParser.DISABLE_ANSI + "=(true|false)", Strings.EMPTY)
              .replaceAll(PatternParser.NO_CONSOLE_NO_ANSI + "=(true|false)", Strings.EMPTY);

To avoid false positive warning messages, aren't we need to replace also the commas with Strings.empty()?
for example: if Options[1] = "noConsoleNoAnsi=false, disableAnsi=false, LOGBACK", string will be ", , LOGBACK".

So I'd suggest to tweak the match pattern to looks like the following:

final String string = options[1]
        .replaceAll(",?" + PatternParser.DISABLE_ANSI + "=(true|false),?", Strings.EMPTY)
        .replaceAll(",?" + PatternParser.NO_CONSOLE_NO_ANSI + "=(true|false),?", Strings.EMPTY)
        .trim();

@ppkarwasz
Copy link
Contributor

@aawad6,

A while back I added a @UsingStatusListener JUnit 5 extensions, which allows you to capture status logger messages into a ListStatusListener:

    @Test
    @UsingStatusListener
    public void testSomething(final ListStatusListener listener) {
        ...
        assertThat(listener.findStatusData(Level.WARN)).hasSize(1);
        ...
    }

Regarding the "disableAnsi" and "noConsoleNoAnsi" keys, we could probably deal with them better. E.g. instead of removing them through a regex, we can add them to the dontEscapeKeys and deal with them in the HighlightConverter constructor.

Copy link
Contributor

@ppkarwasz ppkarwasz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apart from the comment below, it looks good to me.

@ppkarwasz ppkarwasz added this to the 2.20.1 milestone Aug 29, 2023
@vy vy self-assigned this Oct 5, 2023
@vy vy added the enhancement Additions or updates to features label Oct 5, 2023
@vy
Copy link
Member

vy commented Oct 5, 2023

Thanks for the improvement @aawad6, much appreciated! 🙇 Merging it...

@vy vy merged commit 331a8a6 into apache:2.x Oct 5, 2023
7 checks passed
ppkarwasz pushed a commit that referenced this pull request Oct 5, 2023
vy pushed a commit that referenced this pull request Oct 6, 2023
vy added a commit that referenced this pull request Oct 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Additions or updates to features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants