Skip to content

Conversation

@lzaoral
Copy link
Member

@lzaoral lzaoral commented Jul 20, 2021

This is just a draft to see if there are no major issues. I'll add tests if everything will look alright.

Format example:

{
    "msg-filter" : [
        {
            "checker" : "DIVINE|SYMBIOTIC",
            "regexp" : "memory"
        },
        {
            "checker" : "COMPILER_WARNING",
            "regexp" : "called on unallocated object",
            "replace" : "called correctly, no UB here"
        }
    ]
}

@lzaoral lzaoral requested a review from kdudka July 20, 2021 09:24
@kdudka
Copy link
Member

kdudka commented Jul 20, 2021

Nice! My only suggestion is to make MsgFilter::setJSONFilter() catch boost::regex_error to provide a more user-friendly error message when the given regex fails to compile. Instead of:

$ csgrep -f <(echo '{"msg-filter":[{"checker":"","regexp":"*"}]}')
terminate called after throwing an instance of 'boost::wrapexcept<boost::regex_error>'
  what():  The repeat operator "*" cannot start a regular expression.  The error occurred while parsing the regular expression: '>>>HERE>>>*'.
zsh: IOT instruction (core dumped)  csgrep -f <(echo '{"msg-filter":[{"checker":"","regexp":"*"}]}')

$ echo $?
134

... I would expect something like this:

$ csgrep -f <(echo '{"msg-filter":[{"checker":"","regexp":"*"}]}')
/proc/self/fd/11:1: error: Failed to compile regex: The repeat operator "*" cannot start a regular expression.  The error occurred while parsing the regular expression: '>>>HERE>>>*

$ echo $?
1

@praiskup What is your take on this?

@praiskup
Copy link
Member

This is nice, thank you @lzaoral for working on this!

I'd suggest writing some test case for this, and perhaps document the format somewhere so it is easier to use.
Though I think I can personally adapt ;-)

@kdudka
Copy link
Member

kdudka commented Jul 21, 2021

@praiskup Thanks for review! Covering this by tests is already planned as it is written above. And yes, the csdiff project has always been pretty bad regarding documentation of the supported data formats. Any improvements in this regard would certainly be appreciated.

@lzaoral lzaoral force-pushed the json-msg-filter branch 2 times, most recently from d6595e0 to f2c8df6 Compare July 28, 2021 12:49
@lzaoral
Copy link
Member Author

lzaoral commented Jul 28, 2021

Thanks for the reviews @kdudka and @praiskup! I've rebased the PR to include tests, fix the boost::regex_error issue and to document the expected JSON format in respective manual pages.

@lzaoral lzaoral marked this pull request as ready for review July 28, 2021 13:45
@kdudka
Copy link
Member

kdudka commented Jul 28, 2021

Amazing progress. Thank you for working on this!

My only concern is that "checker" : "DIVINE|SYMBIOTIC" currently does not work as expected. It matches the string literally rather than as a regex. This is not your fault. I forgot to say that this is something that yet needs to be implemented. Could you please check pull request #23 which implements it?

If you are fine with it, I would merge #23 first and then rebase this pull request on top of it. There are no conflicts (or test breakage) between these pull requests.

We could probably extend the --filter-file tests to cover the regex-based matching of checkers, too.

@lzaoral
Copy link
Member Author

lzaoral commented Jul 29, 2021

I've rebased the PR to include #23 and #26. Also previously, I forgot to add the new csgrep tests to the corresponding CMakeLists.txt.

edit: fix links to related PRs

@praiskup
Copy link
Member

praiskup commented Aug 3, 2021

This should work for us, thank you @lzaoral!

@lzaoral
Copy link
Member Author

lzaoral commented Aug 3, 2021

I've just changed the documentation to include meaning of the missing replace entry in the JSON format.

@kdudka
Copy link
Member

kdudka commented Aug 3, 2021

Merging, thanks for the contribution!

@kdudka kdudka merged commit fb32526 into csutils:master Aug 3, 2021
@lzaoral lzaoral deleted the json-msg-filter branch August 3, 2021 12:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants