Print name of the Check after printing violation message

[romani]

right now we have format like:

~/Test1.java:5: warning: Uncommented main method found.

it is not clear what Module/Check producing this, it is more actual to naming Checks where format of message is the same for several Check. User do not clearly see what Check config should be adjusted and what Check to search in web documentation , ..... .

PMD has format:
PMD Failure: com/puppycrawl/tools/checkstyle/api/JavadocTokenTypes.java:31 Rule:ExcessiveClassLength Priority:3 Avoid really long classes..

Findbugs format:
com.puppycrawl.tools.checkstyle.checks.UniquePropertiesCheck$UniqueProperties doesn't override java.util.Hashtable.equals(Object) [com.puppycrawl.tools.checkstyle.checks.UniquePropertiesCheck$UniqueProperties] At UniquePropertiesCheck.java:[line 1] EQ_DOESNT_OVERRIDE_EQUALS

should be like (one of) (I am in favour of first or second ):

~/Test1.java:5: Check:UncommentedMain warning: Uncommented main method found.
~/Test1.java:5: warning: UncommentedMain: Uncommented main method found.

~/Test1.java:5: warning: [UncommentedMain] Uncommented main method found.
~/Test1.java:5: warning: Uncommented main method found. [UncommentedMain]

[romani]

@mkordas , @rnveach , @Vladlis , @MEZk , @rdiachenko , @baratali , @sabaka, @tsjensen , @liscju , @Bhavik3 , @attatrol , @oburn please vote on a format or propose your format.

[rnveach]

I vote for the last one. ~/Test1.java:5: warning: Uncommented main method found. [UncommentedMain]
I feel the warning and message are more important. Also when it comes to sorting the display, consecutive messages will be at different positions if the check name is first as we sort by the line number and not by check name.

[mkordas]

👍 for @rnveach proposition

[tsjensen]

I agree with everything that was said, but would like to point out a few additional things:

• Concerning IDE plugins: Eclipse-CS has an option to display the rule name, which is shown at the beginning of the message, e.g.:

  UncommentedMain: Uncommented main method found.
  

  This option should probably be synched with Checkstyle, i.e. we would need a Checkstyle option to turn this on and off, or if we always show the rule name, remove the option from Eclipse-CS. @lkoe What do you think?

• Concerning SonarQube: SonarQube has the rule name from its own knowledge, and displays it in its own GUI, where the message is inserted. So they would have to start parsing the message String to remove the duplication. Again, unless it was an option that they could set when running Checkstyle. @m-g-sonar @mpaladin What do you think?

• Concerning build server plugins: Build servers such as Jenkins and Bamboo parse the XML report. So, in order to retain compatibility, we should not change the format of the XML, but only the message text. (That's what was proposed, so I'm adding this only to make sure.)

Long story short:

• Consider making this behavior configurable.
• Make the change only on the message text.
• 👍 for @rnveach's proposition (warning should be the actual severity)

[romani]

maven-checkstyle-plugin do output like :

[ERROR] My.java:[127,5] (blocks) LeftCurly: '{' at column 5 should be on the previous line.

[romani]

Originally I thought about update of only output that produce our CLI, not a serious update to format.
Update is expected to be at https://github.com/checkstyle/checkstyle/blob/master/src/main/java/com/puppycrawl/tools/checkstyle/DefaultLogger.java#L122 , so lets see who(project) will be affected.

xml format of Checkstyle already have all required information, not changes are required:

/var/tmp $ java -jar checkstyle-6.13-all.jar -f xml -c Test.xml Test.java
<?xml version="1.0" encoding="UTF-8"?>
<checkstyle version="6.13">
<file name="/var/tmp/Test.java">
<error line="4" column="1" severity="error" message="Utility classes should not have a public or default constructor."
 source="com.puppycrawl.tools.checkstyle.checks.design.HideUtilityClassConstructorCheck"/>
</file>
</checkstyle>

output format is:

/var/tmp $ java -jar checkstyle-6.13-all.jar -c Test.xml Test.java
Starting audit...
/var/tmp/Test.java:4:1: error: Utility classes should not have a public or default constructor.
Audit done.
Checkstyle ends with 1 errors.

Here is a clear that output format miss "source" information.

Affected projects:
checkstyle-maven-plugin - will not be affected as it is just redirect output https://maven.apache.org/plugins/maven-checkstyle-plugin/checkstyle-mojo.html#consoleOutput , https://maven.apache.org/plugins/maven-checkstyle-plugin/checkstyle-mojo.html#format

eclipse-cs and other IDE plugins - eclipse-cs do not use DefaultLogger class at all and that is correct, other IDEs also should not use DefaultLogger for reporting.

sonar-checkstyle - does not use DefaultLogger too.

---

So we will not affect most critical plugins, if smb use DefaultLogger it will need to be updated. NO configation is required.

We only need to decide on a format of output.

[romani]

There is one problem of format with "......message [CheckName]" is that any hacking/grep/sed/parsing of output will be complicated as message could contain "[" or "]".

On the other side placing CheckName before message as PMD do distract user from meaningful message, in most cases user do not need to know how Check is named.

---

My decision - I am ok with ~/Test1.java:5: warning: Uncommented main method found. [UncommentedMain] format.

Attention:
we will have small inconsistency between OUTPUT and XML. In XML we will have source="com.puppycrawl.tools.checkstyle.checks.design.HideUtilityClassConstructorCheck" in OUTPUT we will have [HideUtilityClassConstructor] (no namespace and no "Check" suffix). No suffix is required as user need to find by that name documentation on our site easily - http://checkstyle.sourceforge.net/config_design.html#HideUtilityClassConstructor. No namespace to keep output short.

[romani]

@rnveach , @mkordas , @tsjensen , please confirm that you are ok with my last comments to lets us proceed with implementation.

[tsjensen]

Sounds good to me! 👍

[romani]

@MEZk , you can start implementation .

[mkordas]

@romani, I'm OK with your latest comments. However, I have one more proposition, similar to maven-checkstyle-plugin one:

[ERROR] My.java:127:5 LeftCurly: '{' at column 5 should be on the previous line.

Warning level is first, so it's consistent with logging frameworks. There is no strange double colon as in current format (~/Test1.java:5: warning:). Maybe message is a bit more important that check name, but on the other hand sometimes messages are pretty long and check names may be too far away.

[romani]

hmm , good idea , let me think ...
@rnveach , @tsjensen ?

[rnveach]

I am fine with moving warning/error to the first position and in caps.
I still stand by my original proposition that check name should be last.

[m-g-sonar]

Hey guys, from a sonarqube point of view, we will indeed probably have to parse the message to remove the check name, as it will be duplicated with the rule from the UI. The simpler you keep the format, the easier it will be for us to do it.

Now, having the name at the end, as @rnveach proposes, is then maybe the best approach for us. From a feature point of view, I would also consider than then message associated to the error is somehow more important that the rule name itself.

Thank you for the notification! I'll keep an eye on the discussion.

[lkoe]

Sorry for being late to the fray.
From eclipse-cs (Eclipse plugin) point of view I am fine with what is proposed, as long as the decision is being left to the "frontend" - that is DefaultLogger or an IDE plugin etc. (compared to directly adding it to the AuditEvent message, that would be unfortunate)

This way I can still retain the option to toggle this on/off in eclipse-cs, and just need to realign the exact message format to the agreed upon "standard"

[tsjensen]

I am fine with this also.

But, now that I'm thinking about it, it may be helpful to include something in the message that identifies it as a Checkstyle message. Sometimes, people parse build log files to find relevant information, for example for integration purposes. This may not be the best approach, but in the real world, it happens. So we can make life easier by adding Checkstyle, e.g.

[ERROR] Checkstyle My.java:127:5 LeftCurly: '{' at column 5 should be on the previous line.

[romani]

Hey guys, from a sonarqube point of view, we will indeed probably have to parse the message

We are not going to change message of violation, sorry for confusion. We will play with format of console logger only, Sonar does not use it. So Sonar is on save side.

I am fine with what is proposed, as long as the decision is being left to the "frontend" - that is DefaultLogger or an IDE plugin etc

Yes, no changes for AuditEvent class, we are going to have only change only in console output. I expect that nobody is using console to get violation.

Sometimes, people parse build log files to find relevant information

This is the only case where I expect problems on client side, but we will NOT consider it, client has to change its parsing script.

So we can make life easier by adding Checkstyle, e.g.
[ERROR] Checkstyle My.java:127:5 LeftCurly: '{' at column 5 should be on the previous line.

It should be done by console plugins/extensions , we have only simple CLI (this is the main reason of this update) and it will be strange that tool prints its name before each line. I do not know any other tool that do this.

[romani]

[ERROR] My.java:127:5 LeftCurly: '{' at column 5 should be on the previous line.

I am ok with such moving severity to the front.

Maybe message is a bit more important that check name, but on the other hand sometimes messages are pretty long and check names may be too far away.

CheckName will looks good in front only when file path is short, and you have enough space to show message completely. Real example of violation:

[ERROR] ./src/main/java/guava/guava/src/com/google/common/base/Absent.java:127:5 LeftCurly: '{' at column 5 should be on the previous line.

So message will be too far from user attention. Check name is required only then it is not clear who do this violation with desire to reconfigure Check or review its config or get name to get documentation or report issue. In most cases users just follow Checkstyle recommendations.

So format

[ERROR] ./src/main/java/guava/guava/src/com/google/common/base/Absent.java:127:5 '{' at column 5 should be on the previous line. [LeftCurly]

will be read like:
hmm ERROR, this is file I changed , ok , this is tiny mistake. Fixed.
hmm ERROR, this is a file I changed ...., message long is not clear to me (user will read message up to the end), Who is a source of violation ? Here is name, copy-paste of name, goto web for documentation....

[romani]

format for vote:
[ERROR] ./src/main/java/guava/guava/src/com/google/common/base/Absent.java:127:5 '{' at column 5 should be on the previous line. [LeftCurly]

I am +1.
looks like @rnveach is also +1.
looks like @tsjensen is also +1.

@m-g-sonar, @lkoe are not affected.

@mkordas , you are the only left to make a final vote. Please do.

[mkordas]

@romani, +1 from me for your latest proposal

[romani]

@MEZk , you are ok to start implementation.