Contributing (existing) slf4j Logger Checks

[vorburger]

Hello, as per https://lists.opendaylight.org/pipermail/yangtools-dev/2016-May/001391.html, we would be interested in contributing https://git.opendaylight.org/gerrit/gitweb?p=yangtools.git;a=tree;f=common/checkstyle-logging/src/main/java/org/opendaylight/yangtools/checkstyle;hb=HEAD.

This is a collection of individual new Checkstyle Checks related to slf4j logging, helping with enforcement of anti patterns such as:

• Logger must be declared as private static final
• Logger name should be LOG
• LoggerFactory.getLogger Class argument is incorrect
• Logger might be declared only once
• Logger must be slf4j
• Log message placeholders count is incorrect
• Log message contains string concatenation

Q1: Would you welcome this contribution?

Q2: How do we go about this, concretely? Just raise a Pull Request? Do you have any particular "process" ? Any special "quality requirements" ;-) beyond test coverage which we already have

Q3: One of our motivations to contribute this is to avoid having to write/build/distribute a custom Eclipse plugin just for these few Checks. Once this is contributed, would we have to do anything special to get these checks into the sevntu.checkstyle Eclipse plugin, or would you be able to help with that?

Thanks.

[romani]

Q1: Would you welcome this contribution?

Yes, but only if idea is not so specific and are not false-positive prone.
Lets go with case by case.

How do we go about this, concretely?

As usually: open issue with all details, let me review it and confirm. Raise a PR, pass code review, provide testing. Done :) .

Any special "quality requirements"

code review, test reports the same as in checkstyle project. One day all such Checks (after testing period) will be ported to checkstyle .

would you be able to help with that?

sevntu-checkstyle is official sandbox project to let people implement Checks and test them on real code for some time before introducing it to main project and and get storm of issue.
The only restriction is that we will not host crazy ideas like: SQL injection validation, Type related validations .... or any other Check that will provide huge amount of false-positives (due to checkstyle limitations http://checkstyle.sourceforge.net/writingchecks.html#Limitations)

[vorburger]

@romani would you like to have a first round high-level review look at master...vorburger:issue457_slf4jLoggerChecks now?

It still has a number of violations of sevntu's own Checkstyle rules, notably missing JavaDoc etc. but I'd be interested in first basic "worth pursuing" kind of thumbs up from you, before sinking more time into polishing this and raising a PR - maybe you / someone would therefore like to have a look over this already?

[romani]

@vorburger , please create separate issue for each Check you are going to implement and lets discuss its reason and examples of code it will violate (beware that Checkstyle has limitation, not everything is possible to Check).

[vorburger]

@romani sure, let's take this step by step ... would you like to have a first round high-level review look at master...vorburger:issue457a_LoggerDeclarationsCountCheck it still has a number of violations due to missing JavaDoc etc. but I'd be interested in first basic "worth pursuing" kind of thumbs up from you, before sinking more time into polishing this and raising a first of a series of PR - maybe you / someone would therefore like to have a look over this already? Thanks.

[romani]

@vorburger , thanks a for a code but trust my experience in Checks creation.

1. Please stop coding and any other polishing.
2. Create an issue on fist Check and describe its behaviour with examples of code(with violation and without), examples on how configuration will looks like(with all options if any). You will use that text later on in javadoc. Users like examples, so do I :) .
   What I see right now it that "logger" will be such a complicated term to catch reliably without vast of false-positives.
3. As we finish discussion or/and approval of idea , continue "2)" for another Check.

[vorburger]

@romani #461

[koppor]

@vorburger I found this issue while googling for slf4j checkstyle checks

Wanted to check for

LOGGER.warn("No enough closing braces in string: " + s);

which is your case "Log message contains string concatenation".

I absolutely like the other cases, too.

I hope, there could be activity again.