Learn how easy it is to sync an existing GitHub or Google Code repo to a SourceForge project! See Demo

Close

#59 Always put the severity level in logged errors

closed
None
5
2012-10-10
2004-06-19
Henri Tremblay
No

Checkstyle only put the severity in the error log when
the severity is a warning.

Example:
"8:57: warning: Unused parameter 'aUnreadPrimitive'."

For info and error, the severity is not mentionned. You
get

"8:57: Unused parameter 'aUnreadPrimitive'."

The sadly prevent from easily parsing the report when
looking for error or info. I fixed this in the provided diff
(pretty easy, one line of code).

There are two drawbacks to this fix.

First, I don't know how much it is backward compatible
for people having developed a report parser (but I think
it should not change anything. And they probably use
the xml report which contains the severity level anyway).

Second, all check tests should be modified to
include "error: " in the expected result. I am really willing
to provide a diff fixing all tests if needed.

One solution to prevent the drawbacks is to add an
attribute to the checks which will mean to by default
only add "warning:" and if overriden, show all severity.

Discussion

  • Henri Tremblay
    Henri Tremblay
    2004-06-19

    DefaultLogger diff to apply to get the patch

     
  • Lars Kühne
    Lars Kühne
    2004-06-20

    Logged In: YES
    user_id=401384

    The current behaviour resembles the error reporting
    behaviour of many compilers. For example with the gcc
    C-compiler and the JDK javax you'll get error reports like

    x.c:3:10: invalid suffix "x" on integer constant
    x.java:6: ';' expected

    With the drawbacks Henri mentioned, I am not sure we really
    want this patch. On the other hand compilers don't report
    info messages, so I see Henri's point.

    Isn't the correct solution to make this configurable in the
    listener via a command line switch / Ant task attribute?

     
  • Henri Tremblay
    Henri Tremblay
    2004-08-31

    Logged In: YES
    user_id=893525

    I will suggest another fix. I understand the JDK javac style.
    So I'll suggest to at least put ": info" on info as we do for
    warnings. So error won't have nothing and info / warning their
    own tags. And I'll be able to see only the errors by doing:

    maven checkstyle | find /V "info" | find /V "warning"

    Which is a lot better than nothing because currently info and
    error don't have prefix so I can't filter them. And doing this
    you should have a lot less tests to fix.

     
  • Henri Tremblay
    Henri Tremblay
    2004-09-01

    Logged In: YES
    user_id=893525

    The code for my last suggestion would look like that:

            if (!SeverityLevel.ERROR.equals(severityLevel)) {
                sb.append(": " + severityLevel.getName());
            }
    

    to replace

            if (SeverityLevel.WARNING.equals(severityLevel)) {
                sb.append(": warning");
            }
    

    in DefaultLogger.addError

     
  • Henri Tremblay
    Henri Tremblay
    2004-12-07

    Logged In: YES
    user_id=893525

    Any news about this? What do you think of the idea of having
    no header for error and a warning: / info: header for the
    two others?

    It's what the code in my last comment does.

    It won't break your tests and solve the issue correctly.

    The only other solution I can think of will be to have a
    showSeverityLevel property to set. But I prefer the other
    one. It's simpler.

     
  • Oliver Burn
    Oliver Burn
    2004-12-07

    Logged In: YES
    user_id=218824

    I understand the last change suggested by Henri. Seems odd
    to have special handling for WARNING level.

    Question for you Henri, why do you not parse the XML output
    in the first place. Parsing the output of the DefaultLogger
    is not recommended in the first place.

     
  • Henri Tremblay
    Henri Tremblay
    2004-12-07

    Logged In: YES
    user_id=893525

    We do the following. Maven is used to run checkstyle. In
    case of failure we send the report by mail.

    (in fact we send the maven output since we have
    maven.checkstyle.usefile=false)

    Same if it's launched in command line locally. So, for
    example, if one day I want to see only the errors, the
    easiest way will be:

    maven -o checkstyle | find "error"

    or if we implement my last proposal:

    maven -o checkstyle | find /V "info" | find /V "warning"

    And also, in the build failure mail, you can do a find to
    get the errors through the warnings and infos.

    In fact we use a modified checkstyle version at job printing
    error / warning / info (the patch attached in this mail).
    And everybody is happy with it.

    The last thing I tried was the SeverityMatchFilter (ignoring
    bug 1080343). The goal was to have continuous build
    returning only errors and local build to return any
    severity. So at least the received mail will be clean.

    But I can't because as soon as the severity filter in on, I
    cannot bypass it. acceptOnMatch will always be true or
    false. Maybe the filter should do something like:

    <property name="filteredSeverityLevel" value="${checkstyle.filteredSeverityLevel}" default="info,warning,error"/>

    But lets try not to change subject. The thing is that it's
    sometime useful to play on the result file and be able to
    see the severity in it. But I'm not doing it
    programatically. JBCS is correctly using an AuditListener :-)

     
  • Oliver Burn
    Oliver Burn
    2004-12-07

    Logged In: YES
    user_id=218824

    Thanks for the detailed explanation - it helps us understand
    the need/reason. I will take your suggestion below on
    2004-09-01 18:30.

     
  • Logged In: YES
    user_id=746148

    as far as I can see addError() does nothing if event
    severity level is IGNORE (it contains guard if
    if (!SeverityLevel.IGNORE.equals(severityLevel)) {
    )
    So, I suppose that the patch is out of date.
    BTW bug 1080343 which was mentioned as one of reasons of
    this patch has been fixed already.
    So, I close as "out of date" (feel free to reopen it with
    new patch if you still need this fix :)

     
  • D'Arcy Smith
    D'Arcy Smith
    2006-01-19

    Logged In: YES
    user_id=554117

    With GCC 3.4.4 on Cygwin I get the following types of messages:

    x.c:1: error: parse error before "n"
    x.c:3:2: warning: no newline at end of file

    So some compilers now do support both error and warning
    strings.

    It may well be the case that the "plain" output should not
    be parsed, but it is more convenient, for me at least, to
    parse it over the XML output.

    Having error, warning, or info appear would be helpful (or
    at lease warning and info). Having error and info appear
    the same way is not usefule at all.

    For example, trying to grep though the output to find errors
    is impossible to do if you have some checks set to "info".

     
  • Henri Tremblay
    Henri Tremblay
    2007-03-02

    Logged In: YES
    user_id=893525
    Originator: YES

    File Added: DefaultLogger.java

     
  • Henri Tremblay
    Henri Tremblay
    2007-03-02

    New patch for version 4.3

     
    Attachments
  • Henri Tremblay
    Henri Tremblay
    2007-03-02

    Logged In: YES
    user_id=893525
    Originator: YES

    I guess I missed the closing notification... I've just submitted a patch over version 4.3. I'm really sorry, I currently technically can't provide a cvs diff. But I've replaced only 3 lines between 112-114 with one line.

     
  • Oliver Burn
    Oliver Burn
    2010-10-06

    If this is still a requirement, please resubmit the patch.