LocalizedMessage should have only one string identifier of who reproted event

[romani]

found during discussion at #4931.

LocalizedMessage

    public LocalizedMessage(int lineNo,
                            int columnNo,
                            int tokenType,
                            String bundle,
                            String key,
                            Object[] args,
                            SeverityLevel severityLevel,
                            String moduleId,
                            Class<?> sourceClass,

two arguments are used to make identification of who reported and event.

                            String moduleId,
                            Class<?> sourceClass,

Identification argument should be single String type. In our case it will contain simple name of Check class or id from config.

[rnveach]

@romani If we completely drop sourceName, which sourceClass is used for, then suppressions won't be able to suppress by module name anymore if the module defines an ID. This would break compatibility with old suppression files.

See: https://github.com/checkstyle/checkstyle/blob/4c10bc55b4aea0f7e3b95b06d820eedb7c6a6fb2/src/main/java/com/puppycrawl/tools/checkstyle/api/AuditEvent.java#L140-L141 which is used at https://github.com/checkstyle/checkstyle/blob/master/src/main/java/com/puppycrawl/tools/checkstyle/filters/SuppressElement.java#L125

[romani]

If we completely drop sourceName, which sourceClass is used for, then suppressions won't be able to suppress by module name anymore if the module defines an ID.

If user defined a ID, all violations should be suppressed only by this ID, and nothing else. We have ID as non mandatory only because in most cases user use only one instance of it and we did not wanted user to be verbose in config. We should use only IDs, but use class name as default value for ID to keep some compatibility.

If some configs stop working properly, user will find that braking compatibility in release notes.

[romani]

one more nuance with id vs name in suppression. Following commits shows that I did a mistake in upgrading configuration when Check is changed his id to Check name (default identifier): https://github.com/checkstyle/contribution/commit/2ca8010de9df22ac6471e3d7224d7741bde15470 https://github.com/checkstyle/contribution/commit/2d50c3c6e700c6b5ccbd393a31d10cccbc21c978

[rnveach]

@romani What are we calling that field in the violation display that either shows the check name or id. Ex: [ERROR] TestClass.java:3:8: Variable 's' should be declared final. [FinalLocalVariable] What are we calling [FinalLocalVariable]?

Would it make more sense to deprecate check name and/or id in the suppression and make a new field or use one field for the information between the brackets?

For example, why don't we deprecate id and use checks for all violations and the value in the bracket must be the value used for the suppression? This would simplify the suppression configuration and prevent any confusion on using the attribute id or check name. As it is right now, you would have to look in the original configuration to figure out if the violation being displayed is an id or check name, which users may not know offhand especially if the configuration is a global one used by the company or written by a 3rd party (google_checks).

[rnveach]

I'm not sure, but dropping access to the full class name for the id when it is specified might break some plugins like eclipse and maven who use the full class name for its purposes.

https://github.com/checkstyle/eclipse-cs/blob/e9dc6228e0310126df589b04140b0cf895fe3a75/net.sf.eclipsecs.core/src/net/sf/eclipsecs/core/builder/Auditor.java#L337

https://jira.apache.org/jira/browse/MCHECKSTYLE-344

[romani]

We do not drop full class name, it is still should be present in that argument. But if user defined ID, that field contains ID, as it is sign that there are two instances of the same Check, so identification by class name is not valid.

[rnveach]

Yes and that will break compatibility if plugins are always expecting class name and they don't get it when user specifies ID. This is what broke checkstyle maven plugin which has still not been fixed. I don't know the reason why they need full classpath of check always. I am just stating this may break plugins more which will be a problem since they never want to update anymore.

[romani]

it mean such plugins are not working correctly now with multiple instances of the same Check. It is very used model, it is even in google_checks.xml . But I agree to put "breaking compatibility" label to issue.