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

Log severity for all audit events, not just warnings #67

Closed
richarddbarnett opened this issue Nov 18, 2013 · 10 comments
Closed

Log severity for all audit events, not just warnings #67

richarddbarnett opened this issue Nov 18, 2013 · 10 comments
Assignees
Labels
Milestone

Comments

@richarddbarnett
Copy link

DefaultLogger.addError() only logs AuditEvent.severityLevel if it's WARNING.
This makes it hard to find events of severity ERROR in eg mvn checkstyle:check output.
Log severityLevel in all cases.

@richarddbarnett
Copy link
Author

Patch (based on 5.3) that unfortunately breaks many unit tests:

diff -r 1b7a252d5e1e src/checkstyle/com/puppycrawl/tools/checkstyle/DefaultLogger.java
--- a/src/checkstyle/com/puppycrawl/tools/checkstyle/DefaultLogger.java Tue Oct 19 18:56:17 2010 +1000
+++ b/src/checkstyle/com/puppycrawl/tools/checkstyle/DefaultLogger.java Wed Feb 22 11:38:12 2012 +1100
@@ -109,9 +109,7 @@
             if (aEvt.getColumn() > 0) {
                 sb.append(':').append(aEvt.getColumn());
             }
-            if (SeverityLevel.WARNING.equals(severityLevel)) {
-                sb.append(": warning");
-            }
+            sb.append(": ").append(severityLevel.getName());
             sb.append(": ").append(message);
             mErrorWriter.println(sb.toString());
         }

@romani
Copy link
Member

romani commented Nov 20, 2013

valid point, patch is correct on 5.7-Snapshot, but ...

DefaultLogger is used in UTs, so it have to be updated to have property that will manage printing of Severity - "boolean printSeverity", by default for UTs it will be disabled to satisfy all existing UTs. In class that option will be "true" by default.
WriteTagCheckTest.testTagSeverity() - is only UT that would fail but it have to be updated to create Logger (BaseCheckTestSupport have to be extended with

createChecker(....., boolean printSeverity) {
....
c.addListener(new BriefLogger(mStream, printSeverity));
}

for himself with printing severity level.

@richarddbarnett, if you need this fix .... please follow instructions above in other case wait .

@alexkravin
Copy link
Contributor

If I understood everything correct..

In all places except for UTs this option is used with its default value (true), so new option was added to DefaultLogger's ctors and all calls of these ctors are updated with true value, except for BaseCheckTestSupport.

New createChecker() and verify() methods were introduced in BaseCheckTestSupport to support the setting of new option.

@romani
Copy link
Member

romani commented Mar 15, 2015

fixed, will be in 6.5

@romani
Copy link
Member

romani commented Mar 18, 2015

compatibility problem should be resolved https://groups.google.com/forum/#!topic/checkstyle-devel/3jJ4cCQ3sM0

I revered as commit from master, now checkstyle-tester project (https://github.com/checkstyle/contribution/tree/master/checkstyle-tester) works fine

@romani romani reopened this Mar 18, 2015
@alexkravin
Copy link
Contributor

Corresponding instantiation just have to be updated according to new params

@romani
Copy link
Member

romani commented Mar 18, 2015

We can not wait for plugin to update that, maven plugin is still on very old version.
Please provide a solution to resolve that issue and do not brake compatibility.

@MEZk
Copy link
Contributor

MEZk commented Sep 9, 2015

@romani
Is it a good idea to implement setter for printSeverity variable instead of initialization in constructor?

@romani
Copy link
Member

romani commented Sep 9, 2015

Not the best, i think that one mor c-tor will be better solution

MEZk added a commit to MEZk/checkstyle that referenced this issue Sep 10, 2015
MEZk added a commit to MEZk/checkstyle that referenced this issue Sep 10, 2015
MEZk added a commit to MEZk/checkstyle that referenced this issue Sep 10, 2015
@romani romani added this to the 6.11 milestone Sep 10, 2015
@romani
Copy link
Member

romani commented Sep 10, 2015

merged

@romani romani closed this as completed Sep 10, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants