Suppression for violations

[romani]

Thanks for good tool.

FYI: checkstyle/checkstyle#1217

please think about some suppressions mechanism, that is good to forbid usage on jdk-system-out in all code except for class that is responsible for CLI.

[uschindler]

Hey,
forbidden-apis 1.8 has support for this! You can use annotations (class/runtime retention) to suppress forbidden on whole classes, methods or fields (limited usage).

If you don't want to use the official annotation, you can define your own one and pass the class name as parameter to forbidden-apis: http://jenkins.thetaphi.de/job/Forbidden-APIs/javadoc/check-mojo.html#suppressAnnotations

One example is Elasticsearch: https://github.com/elastic/elasticsearch/blob/fd6112171d281cd904adedd2757e1f928edd5116/pom.xml#L723

One example about suppressing: https://github.com/elastic/elasticsearch/blob/4c981ff4bfc250080d521af105b5e8589c9fc517/core/src/main/java/org/elasticsearch/common/logging/log4j/ConsoleAppender.java

Here is the annotation: https://github.com/elastic/elasticsearch/blob/4c981ff4bfc250080d521af105b5e8589c9fc517/core/src/main/java/org/elasticsearch/common/SuppressForbidden.java

[uschindler]

Duplicate of #34 and #53

[romani]

May be I am alone who think like that ..... but I do not like when production code is polluted with @SuppressXXXXXXX annotations caused by numerous analysis tools, code already have number of nuances and details and that is not required for runtime.

It would be good to somehow define suppression/configration in standalone file or inline in pom.xml.
Example of suppression: https://github.com/checkstyle/checkstyle/blob/master/config/findbugs-exclude.xml , https://github.com/checkstyle/checkstyle/blob/master/config/pmd.xml#L51 , https://github.com/checkstyle/checkstyle/blob/master/config/suppressions.xml.

Keeping all that stuff in code would be disaster.

[uschindler]

As alternative, you can exclude files from checking via pom.xml: http://jenkins.thetaphi.de/job/Forbidden-APIs/javadoc/check-mojo.html#excludes

You give the class files as path. The backside is, that it does only work on class file level.

Keeping all that stuff in code would be disaster.

Lucene and Elasticsearch each have only like 10 annotations like this in the whole code base, so I don't think its a desaster. It makes it easier to read, because you can see the "reason" message in the annotation, too.

[romani]

because you can see the "reason" message in the annotation

you do not need to see reason of what was already done and approved. Engineer need to see a reason of why his new changes are not allowed. The most important is that - it should be that simple to introduce suppression (by annotations it is easy). Example: junior introduce a code and put a suppress annotation on it, senior could easily miss that among all other changes.
By having special pace for suppression - it is so easy to catch that change in a diff and question that suppression. In my practice, contributors are afraid to extend that suppressions and start discussion before sending PR with bad change.
One more point - if tool give a lot of false-positives over the code that mean that tool is not used right ..... smth should be done in design to have exclude a very rare event.

As alternative, you can exclude files

thanks a lot, it will work for us. I hope you will see your library in our build soon.

[uschindler]

Hi @romani,
I agree with your explanation. My personal problem with enabling the @SuppressForbidden annotation were the same, but the Open-Source community wanted to have it that way. I think it depends on your workflow how you submit code. If a committer has to modify a separate file to make a checker run pass, it gets more visibility than a maybe hidden @SuppressForbidden annotation.

In Apache Lucene and Elasticsearch we want to see a reason, why some code was marked by an annotation (e.g., why a test was @ignore'd). So this is just our coding guideline, so later committers have a chance to understand without reading issues or reviewing commit logs. A simple comment would be fine, too, but a "requirement" on the annotation enforces to write something.

One more point - if tool give a lot of false-positives over the code that mean that tool is not used right ..... smth should be done in design to have exclude a very rare event.

Yes, if you use system-out forbids and you mark tons of classes as @SuppressForbidden you are doing something wrong. One example to solve this and reduce the number of suppressed code:

• have a base class that all CLI tools extend from
• the base class has some methods to do screen output
• the base class has the screen output methods @SuppressForbidden (and only those)
• all CLI tools should extend those CLI base class

In any case, I can have a look into allowing to pass a "signature" list (same format like the forbidden signatures) with classes/methods that should not be scanned for violations. Let's open a separate issue about that, this one was too "generic" (this is why I closed is as duplicate).

[dweiss]

I personally prefer when these annotations are placed directly on the target. No surprises and a single point of failure -- you remove a file, you remove the suppression as well, etc.

It's really subjective.