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:
[email protected]
With regards,
Apache Git Services
