Add SuppressionAnnotationFilter

[attatrol]

A new filter is added. It satisfies issue #1340.
checkstyle/checkstyle#1340

It suppresses events generated from code elements annotated by user defined annotations.

Here is a link to the xdocs entry for the filter:
https://gist.github.com/attatrol/699fd9863de8730dfc30

I was to add small changes to .travis.yml from the original, because somehow previous version errored:
https://travis-ci.org/attatrol/sevntu.checkstyle/jobs/72255048

[ERROR] Failed to execute goal org.eluder.coveralls:coveralls-maven-plugin:2.2.0:jacoco (default-cli) on project sevntu-checks: Processing of input or output data failed: Report submission to Coveralls API failed with HTTP status 422: Unprocessable Entity (Couldn't find a repository matching this job.) -> [Help 1]


[romani]

postponed to 1.14

[romani]

@attatrol , can we finish this PR ?

[romani]

@attatrol, do you plan to finish this PR ?

If you have no time, just let me know, I find another contributor to finish.

[attatrol]

@romani,
i will focus on it from now.

[attatrol]

@romani
i fixed everything you pointed on.

[romani]

@attatrol ,

 * &nbsp;&nbsp;&nbsp;&nbsp;...<br/>
 * &nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp

please look at others Checks, we use {code} and <pre> tags to keep formatting as is.

private static final boolean DEFAULT_MODIFIERS_EXCLUDED = true;

I do not see any reason in field, please inline its value.

private transient List checkRegexp

Why is it transient ?

DetailAST getEndNode(DetailAST node) {

make a single exit point from method.

[attatrol]

@romani,
1)

please look at others Checks, we use {code} and < pre > tags to keep formatting as is.

i did as you said, and now i get 7 errors from JavadocStyle for the parts surrounded by {code} tag.

[ERROR] src/main/java/com/github/sevntu/checkstyle/filters/SuppressionAnnotationFilter.java:56,4 JavadocStyle: Extra HTML tag found:
[ERROR] src/main/java/com/github/sevntu/checkstyle/filters/SuppressionAnnotationFilter.java:68,8 JavadocStyle: Extra HTML tag found:
[ERROR] src/main/java/com/github/sevntu/checkstyle/filters/SuppressionAnnotationFilter.java:70,4 JavadocStyle: Extra HTML tag found: }
[ERROR] src/main/java/com/github/sevntu/checkstyle/filters/SuppressionAnnotationFilter.java:86,9 JavadocStyle: Extra HTML tag found:
[ERROR] src/main/java/com/github/sevntu/checkstyle/filters/SuppressionAnnotationFilter.java:88,4 JavadocStyle: Extra HTML tag found: }
[ERROR] src/main/java/com/github/sevntu/checkstyle/filters/SuppressionAnnotationFilter.java:103,9 JavadocStyle: Extra HTML tag found:
[ERROR] src/main/java/com/github/sevntu/checkstyle/filters/SuppressionAnnotationFilter.java:105,4 JavadocStyle: Extra HTML tag found: }

Please confirm whether it is my fault or it is a bug.

private static final boolean DEFAULT_MODIFIERS_EXCLUDED = true;

I do not see any reason in field, please inline its value.

Done.

private transient List checkRegexp

Why is it transient ?

The modifier was "inherited" from the SupressionCommentFilter when i was newbie in Java. It was deleted.

DetailAST getEndNode(DetailAST node) {

make a single exit point from method.

Done.

[romani]

Travis need to be fixed - https://travis-ci.org/sevntu-checkstyle/sevntu.checkstyle/jobs/109062733#L512

Nothing is bad in your code https://blogs.oracle.com/darcy/entry/javadoc_tip_code_and_literal .
We need to do workaround with <pre><code> if JavadocStyle is not ready to support such formatting, RegExp based Javadoc Checks are extremely buggy.

[attatrol]

@romani ,
i fixed javadoc problem.

[romani]

DetailAstRootHolder.java

/** Weak reference on the root of the current AST. */
+    private static final ThreadLocal<DetailAST> ROOT 

So, weak reference or TreadLocal ?

public int[] getDefaultTokens() {

please override all getXXXXXXTokens methods

---

SuppressionAnnotationFilter.java

 *     <module name="DetailASTRootHolder"/>
 *     ...}

weird or misplaced "...}" . The same for other examples.

name will satisfy.

name will match.

permittedChecks

rename to "checkName" (without "s", as it is regexp and not a collection)

Please make example for annotation @Generated , https://docs.oracle.com/javase/7/docs/api/javax/annotation/Generated.html . Users already wants to suppress all Checks in generated classes by such annotation.

modifiersExcluded

I do not understand reason of this option, please update Javadoc example(config + code sample) to make it clear.

annotationNamesSet

please remove "Set" from name. Please do this for other fields too.

    /** Excludes annotations from check if set true. */
    private boolean modifiersExcluded = true;

annotations vs modifiers ?! Terminology has to be consistent.

setAnnotationName(

should be with "s" - setAnnotationNames(

//annotationNameSet.add(string);

remove dead code.

            final int lastIndexOfPoint = string.lastIndexOf('.');
            annotationNamesSet.add(string.substring(lastIndexOfPoint + 1));

make a static function for this, name of method will explain meaning of it.

public List getCheckRegexp() {

why public ?

        for (String string : permittedChecksSet) {
            try {
                checkRegexp.add(Pattern.compile(string));
            }
            catch (final PatternSyntaxException ex) {
                throw new CheckstyleException("unable to parse expanded comment " + string, ex);

permittedChecksSet --> string --> 'expanded comment' !!!??

please make good names for all terms

private String getAnnotationName(DetailAST annotation) {

please make it static, please verify all other methods to make them static.

private void addSuppressRange(DetailAST node) {

please move ranges.add(suppressRange); to upper level of method call stack. rename method, make it static.

DetailAST getStartNode(DetailAST node) {

make it private and static and single return.

private boolean doNotRoundStartLineNumber(DetailAST node) {

avoid "NOT" in method names as much as possible. please rename and refactor.

private boolean isPermittedCheck(AuditEvent event) {

do not invent terminology use "match", method inner code already tell you this.

[attatrol]

@romani ,
1), 2) Done
3)

 *     <module name="DetailASTRootHolder"/>
 *     ...}

weird or misplaced "...}" . The same for other examples.

i removed {@code } tags, now javadoc structure is consistent with nearby checks' javadocs.

4. Done

permittedChecks

rename to "checkName" (without "s", as it is regexp and not a collection)

Actually it is a collection. It is used with regexp list checkRegexp to provide user with another way of check names definition. See paragraphs 3 and 4 of javadoc.

Please make example for annotation @Generated , https://docs.oracle.com/javase/7/docs/api/javax/annotation/Generated.html . Users already wants to suppress all Checks in generated classes by such annotation.

Javadoc paragraph 2 updated. By the way this annotation was the impetus for this filter creation.

modifiersExcluded

I do not understand reason of this option, please update Javadoc example(config + code sample) to make it clear.

Javadoc paragraph 5 updated.

8. • 11. Done

            final int lastIndexOfPoint = string.lastIndexOf('.');
            annotationNamesSet.add(string.substring(lastIndexOfPoint + 1));

make a static function for this, name of method will explain meaning of it.

method getSimpleName created

13. • 15. Done

private void addSuppressRange(DetailAST node) {

please move ranges.add(suppressRange); to upper level of method call stack. rename method, make it static.

Done, but it is impossible to make it static as it calls nonstatic method.

DetailAST getStartNode(DetailAST node) {

make it private and static and single return.

I made it private and single return, but it directly asks for a field, so i chose to leave it nonstatic.

private boolean doNotRoundStartLineNumber(DetailAST node) {

avoid "NOT" in method names as much as possible. please rename and refactor.

Done, new method name is columnIndexIsNecessary

private boolean isPermittedCheck(AuditEvent event) {

do not invent terminology use "match", method inner code already tell you this.

Done, new method name is matchCheck

[romani]

DetailAstRootHolder.java

private static final ThreadLocal<DetailAST>

Checkstyle is single thread application by design. So why ThreadLocal is required ?

permittedChecks

rename to be "checkNames"

---

BaseCheckTestSupport.java

protected PrintStream getPrintStream()

Weird formatting.

[attatrol]

@romani ,
20)

private static final ThreadLocal<DetailAST>

Checkstyle is single thread application by design. So why ThreadLocal is required ?

I thought it was a good idea to be consistent with analog class.

permittedChecks

rename to be "checkNames"

Done

protected PrintStream getPrintStream()

Weird formatting.

Whole class is wierd formatted, i made this method consistent with the rest of the class.

[romani]

1. To configure a filter to suppress audit events for a certain annotated entities

All events ? I did not found "certain" in config. What is "certain" ? Be exact.
Please provide code with violated line and correct line.
Please update all other examples. It is not clear how Filter should be used.

must be found in name of the check * in order to pass through

Filter is suppression(hiding), annotation is suppression marker, but why check is smth is NOT smth that will be suppressed ?

permitted * check's names

permitted ?

5.In context of this filter annotations themselves, javadocs, access modifiers
...
name="modifiersExcluded" value="false"

I do not undrstand reason of this option at all. Please provide example why it is matter.
We need to think a about name that have negative meaning: Exclude==false is hard to read.

public void setAnnotationNames(String... annotations) {
for (String str : annotations) {

annotations --> anootationNames , str--> name.
Please review other code and do the same in "setCheckNames", "matchesCheck".

protected void finishLocalSetup()
throws CheckstyleException {

make a single line.
Lets do patern compilation it setter and remove this method completely.

if (event.getLocalizedMessage() == null
if (currentRoot == null)

Please let me know when message could be null, and when currentRoot is null ?

Produces simple form from the annotation name.

simple form ?

private void resetLocalData(DetailAST currentRoot) {
final DetailAST localRoot = currentASTRootRef.get();
if (currentRoot != localRoot) {

move IF to calling method. If you name method "reset....." do reset. In other case name method "resetLocalDataIfPossible". Name method in exactly as what it is doing.

private void traverseAst(DetailAST currentRoot)

currentRoot --> node

[romani]

I thought it was a good idea to be consistent with analog class.

It was there from the beginning.
https://github.com/checkstyle/checkstyle/blame/262209477f39964a63103613f3e2c985da083371/src/checkstyle/com/puppycrawl/tools/checkstyle/checks/FileContentsHolder.java
It is a mistake. We will fix this - checkstyle/checkstyle#2992.

[romani]

@attatrol , is there any change to finish this PR ?
Such functionality is really in demand.
If you do not have time to finish it, please let me know, I will try to find another contributor to finish it.

[attatrol]

@romani,
It is ok if someone else finishes it, i simply have no time due to university classes. But if you don't find anyone i will return to it in late June.

[romani]

thanks a lot for answer.

[romani]

we try to get gid of all Holders in Checkstyle now , original issue in checkstyle repo might be fixed by new Xpath Filter - see checkstyle/checkstyle#1340 (comment)

there is not future in this update and it is abandoned already.