[GitHub] rnveach commented on a change in pull request #7: [MCHECKSTYLE-357] - Allow inline configuration for reporting

[GitBox <gi...@apache.org>]

rnveach commented on a change in pull request #7: [MCHECKSTYLE-357] - Allow inline configuration for reporting
URL: https://github.com/apache/maven-checkstyle-plugin/pull/7#discussion_r241374703
 
 

 ##########
 File path: src/main/java/org/apache/maven/plugins/checkstyle/AbstractCheckstyleReport.java
 ##########
 @@ -73,6 +74,10 @@
 
     protected static final String JAVA_FILES = "**\\/*.java";
 
+    private static final String CHECKSTYLE_FILE_HEADER = "<?xml version=\"1.0\"?>\n"
+            + "<!DOCTYPE module PUBLIC \"-//Puppy Crawl//DTD Check Configuration 1.3//EN\"\n"
+            + "        \"http://www.puppycrawl.com/dtds/configuration_1_3.dtd\">\n";
 
 Review comment:
   Hello I am with Checkstyle team. I just wanted to point out some things I noticed after thinking about this PR some more.
   
   1) Placing the DTD header here will always force the configuration to this version. If Checkstyle increases version and adds new functionality, maven-checkstyle won't be able to accept it until this is updated and users may not know about this limitation unless it is documented somewhere.
   
   1a) My only concern with this is that maven plugin has been somewhat slow to release updates. it took 3 years just to get 3.0 released, and some users are asking in another issue for another fix to be released. See https://issues.apache.org/jira/browse/MCHECKSTYLE-344 .
   
   2) URL has changed to `https://checkstyle.org/dtds/configuration_1_3.dtd` recently. As long as the public ID remains as it is, which is backwards compatible, Checkstyle will load the internal DTD and not require an internet connection. This should be the case for older versions, but I am not that familiar with all of them.
   
   3) Compatibility with future and older versions may be an issue because of all the reasons I stated above. 6.18 should be able to support 1.3 configuration but it may cause issues with checkstyle versions older then 6.18 as users may try to use configuration options that aren't available to them in that version. If configuration version is ever updated, we may not support `Puppy Crawl` public ID anymore.
   
   I don't see where your testing specifies what versions you test with, but it may be a good idea to expand it to more versions other than 6.18 if it isn't already.
   
   My only idea for continuing with this PR is maybe give the user the option specify configuration version and DTD ID, instead of hardcoding it. The default can be what 6.18 supports.
   
   These are just my thoughts. Take them as you please.

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services