Issue #20: Configuration Component #43

Luolc · 2017-06-29T20:45:28Z

Missing the output to file function currently.

rnveach

I noticed there was no discussion in issue on how we were going to implement this before coding, so we are back to long discussions in PR.

rnveach · 2017-06-29T20:52:38Z

src/test/java/com/github/checkstyle/regression/configuration/ConfigGeneratorTest.java

+    @Test
+    public void testGenerateConfigTextWithEmptyModuleInfos() throws Exception {
+        final String excepted = ""
+                + "<?xml version=\"1.0\" encoding=\"UTF-8\"?>\n"


Let's put expected XML files in external file similarly we do for main Checkstyle project.

rnveach · 2017-06-29T20:57:07Z

src/main/java/com/github/checkstyle/regression/configuration/ConfigGenerator.java

+        final Element moduleNode = document.createElement(ELEMENT_MODULE);
+        moduleNode.setAttribute(ATTR_NAME, moduleInfo.name());
+
+        for (ModuleInfo.Property property : moduleInfo.properties()) {


Please remove and ignore all property code until #5 .

rnveach · 2017-07-02T19:16:55Z

config/pmd.xml

@@ -189,6 +189,8 @@
    <exclude name="MethodArgumentCouldBeFinal"/>
    <!--not configurable, decreases readability-->
    <exclude name="UseStringBufferForStringAppends"/>
+    <!--decreases readability when writing a muti-line string-->
+    <exclude name="AddEmptyString"/>


XML strings should be in their own file, so this can be removed.

rnveach · 2017-07-02T19:17:28Z

src/main/java/com/github/checkstyle/regression/configuration/ConfigGenerator.java

+
+    /** The config template. */
+    private static final String BASE_CONFIG = ""
+            + "<?xml version=\"1.0\" standalone=\"yes\"?>"


This should be in it's own file and not hard coded into source.

rnveach · 2017-07-02T19:19:08Z

src/main/java/com/github/checkstyle/regression/configuration/ConfigGenerator.java

+            + "  <property name=\"charset\" value=\"UTF-8\"/>\n"
+            + "  <property name=\"severity\" value=\"warning\"/>\n"
+            + "  <property name=\"haltOnException\" value=\"false\"/>\n"
+            + "  <module name=\"BeforeExecutionExclusionFileFilter\">\n"


Add comments founded in regression tool's XML.
https://github.com/checkstyle/contribution/blob/master/checkstyle-tester/my_check.xml#L10

rnveach · 2017-07-02T19:20:56Z

src/main/java/com/github/checkstyle/regression/configuration/ConfigGenerator.java

+        final Document document;
+
+        try {
+            document = DocumentBuilderFactory.newInstance().newDocumentBuilder()


This XML process shouldn't do any validating of DTD or request an internet connection to process.
Some users like to do regression solo when they are on a plane or such, so we need to limit internet connectivity when we can.

Do do mean that document = DocumentBuilderFactory.newInstance().newDocumentBuilder()... this line shouldn't do internet request? I switch off my internet connection and run the UTs of this class and everything is fine. Is that OK?

That is fine. I just wanted you to be aware and verify. I am not familiar with the document builder.

rnveach · 2017-07-02T19:21:39Z

src/main/java/com/github/checkstyle/regression/configuration/ConfigGenerator.java

+                returnValue = node;
+                break;
+            }
+        }


empty line after for.

rnveach · 2017-07-02T19:24:50Z

src/main/java/com/github/checkstyle/regression/configuration/ConfigGenerator.java

+
+    /** The "doctype-public" value of the config. */
+    private static final String DOCTYPE_PUBLIC =
+            "-//Puppy Crawl//DTD Check Configuration 1.3//EN";


Can we add a test to verify this doctype is latest version from Checkstyle release?
If this changes, I want us to be required to update this and verify it has no repercussions on regression.

latest version from Checkstyle release

Is it OK to grab the latest version from this https://github.com/checkstyle/checkstyle/blob/master/config/checkstyle_checks.xml? Is it required to be checkstyle release version but not SNAPSHOT version?

We can try this but internet connection may be an issue as travis uses anonymous user to access github.
Snapshot might be an issue if it isn't released, but I am fine testing this out and removing it if there is a problem.

So are we able to find the latest version of it from somewhere else?

without an internet connection, no. Github is the only place that has the latest information and it is stored in a separate repo.

Add a testDtdVersion in the test class. CI goods fine.

rnveach · 2017-07-02T19:28:33Z

src/main/java/com/github/checkstyle/regression/configuration/ConfigGenerator.java

+            + "    <property name=\"fileNamePattern\" value=\"module\\-info\\.java$\" />\n"
+            + "  </module>\n"
+            + "  <!-- Contents below are generated by regression-tool. -->\n"
+            + "  <module name=\"TreeWalker\"/></module>";


The tool can't add TreeWalker for us?
If so, please explain the reason. Someday we may add other TreeWalkers. Just like we don't embed these modules into our extract, I would like to avoid embedding into other areas.

Someday we may add other TreeWalkers

I don't know whats the meaning of this, maybe smth like MutiThreadTreeWalker in the future?

I think by now, it is OK to let the tool generate TreeWalker dynamically. But there would be some other questions:
If we assume that the module would have whatever parent(not just Checker and TreeWalker), so how to decide where to put the parent module? Should we gain the info from the module extract info map? If yes, then configuration component would depend on module component, otherwise we would have to pass the whole info map to configuration component, but I don't think it is a good idea.

I don't know whats the meaning of this, maybe smth like MutiThreadTreeWalker in the future?

TreeWalker = Java grammar.
If we add XML grammar, we will need something like XmlTreeWalker, etc.
MutiThreadTreeWalker is another possibility.

configuration component would depend on module component

All components already rely on previous component. Next components input is previous component's output. I don't see any reason to embed the 2 together right now.

how to decide where to put the parent module? Should we gain the info from the module extract info map?

Module extract info should already have this information ready for us to use.
This is why we have parent and it is just String.
Checker has no parent, so it is always root module. TreeWalker has parent Checker, so it goes as a sub-module of it. All checks have TreeWalker as parent, so they are sub-modules of it.
The extract can be looped through to understand hierarchy. If some hierarchy information is missing, we should throw an exception and say we can't continue.

I think by now, it is OK to let the tool generate TreeWalker dynamically

But will it still require hardcoding of hierarchy somewhere? If so, let's move this to a new issue and keep code as is.

But will it still require hardcoding of hierarchy somewhere?

Yep, by now we need to know the fact that "TreeWalker's parent is Checker", and that info should be gained through the module extract info map.

Please create new issue then. We should avoid hardcoding things that can change in a single PR.

Please create new issue then. We should avoid hardcoding things that can change in a single PR.

#47 for this

rnveach · 2017-07-03T16:51:15Z

@Luolc appveyor should pass.

Luolc · 2017-07-03T17:00:46Z

@rnveach Seems like a CRLF problem again, I am fixing it.

https://github.com/checkstyle/contribution/blob/master/checkstyle-tester/my_check.xml#L4
https://github.com/checkstyle/checkstyle/blob/master/config/checkstyle_checks.xml#L4

http://checkstyle.sourceforge.net/dtds/configuration_1_3.dtd and http://www.puppycrawl.com/dtds/configuration_1_3.dtd, which one should we use? They are different in checkstyle project and contribution repo.

rnveach · 2017-07-03T17:33:36Z

which one should we use?

We should always go by main project. We are forced to update and use things there.
Contribution is a 3rd party project that uses whatever they want. They aren't forced to use latest.

Luolc · 2017-07-03T17:52:23Z

@rnveach I updated the url to http://checkstyle.sourceforge.net/dtds/configuration_1_3.dtd. CRLF is fixed and a test of dtd version is added.

rnveach

I don't see anything else.

rnveach · 2017-07-03T17:59:51Z

src/main/java/com/github/checkstyle/regression/configuration/ConfigGenerator.java

+
+        final Node checkerNode = getCheckerModuleNode(document);
+        final Node treeWalkerNode = getTreeWalkerModuleNode(document);
+        for (ModuleInfo moduleInfo : moduleInfos) {


empty line before for.

rnveach · 2017-07-03T18:02:09Z

src/main/java/com/github/checkstyle/regression/configuration/ConfigGenerator.java

+        final Transformer transformer = createXmlTransformer();
+        final StringWriter writer = new StringWriter();
+        transformer.transform(new DOMSource(document), new StreamResult(writer));
+        return writer.getBuffer().toString().replaceAll("\r\n", "\n");


.replaceAll("\r\n", "\n");

Does this really need to be in production code? We are no longer doing any processing on this and Checkstyle/XML doesn't care about whitespace.
I assume this was added just to make tests pass, so why don't we keep it in test code.

rnveach · 2017-07-03T18:03:54Z

src/main/java/com/github/checkstyle/regression/configuration/ConfigGenerator.java

+import com.github.checkstyle.regression.data.ModuleInfo;
+
+/**
+ * Generates the config XML and output it to a specific file.


You return a String, not a file.
If this component is returning a file, the public method needs to change to return a file. Otherwise this javadoc needs to change.

Missing the output to file function currently.

I mentioned this at the first post of this PR.

I am not sure which one(string or output to a file) is better. Personally I prefer output it to file, but I am also fine with a String. So I just left this when I first push this PR. What't your opinion?

Returning string to me seems weird as we will not be doing any more String manipulation and just output it to a file anyways. I would prefer it to return a file.

Done. generateConfig creates and output the xml plain text to a file. Related UTs are updated as well to directly compare whether two files are the same.

rnveach · 2017-07-03T18:05:41Z

src/main/resources/com/github/checkstyle/regression/configuration/base_config.xml

+  </module>
+
+  <!-- Contents below are generated by regression-tool. -->
+  <module name="TreeWalker"/></module>


Move </module> to next line.
You don't write code like:

void method() { if (condition) {} }

I don't like this either, but there would be a strange indention problem if I put </module> to next line.

i.e. The generated XML would be smth like this. the last second line has wrong indention.

<?xml version="1.0" encoding="UTF-8"?> <!DOCTYPE module PUBLIC "-//Puppy Crawl//DTD Check Configuration 1.3//EN" "http://checkstyle.sourceforge.net/dtds/configuration_1_3.dtd"> <module name="Checker"> ...  <module name="TreeWalker"/> <module name="FileLengthCheck"/> </module>

You could have a try to put that to next line, I assume testGenerateConfigTextWithEmptyModuleInfos and testGenerateConfigTextWithCheckerParentModule would fail due to that indention problem

We shouldn't care about generated files as they are out of our control. I'm suprised that it even kept the comments.
Let's only concern ourselves with our files for now.

rnveach · 2017-07-04T17:40:00Z

src/main/java/com/github/checkstyle/regression/configuration/ConfigGenerator.java

+    }
+
+    /**
+     * Generates the config XML file from the given module inofs.


inofs => infos I assume?

rnveach · 2017-07-04T17:43:17Z

src/main/java/com/github/checkstyle/regression/configuration/ConfigGenerator.java

+            throws IOException, TransformerException {
+        final File file = new File(path);
+        Files.write(file.toPath(), generateConfigText(moduleInfos)
+                .getBytes(Charset.forName("UTF-8")), StandardOpenOption.CREATE_NEW);


Shouldn't we be using TRUNCATE_EXISTING instead of CREATE_NEW. We were told a path to place config, we should delete it if it already exists and not fail causing user to erase it manually.

Done. I used CREATE_NEW for I was worried that user might accidentally write a path to another existing important file. But it is not common, I am fine with erasing.

File name should be completely under our control. If user wants to save it, they should it move outside of our path. I don't expect users to have to supply a unique file path.

rnveach · 2017-07-04T17:48:06Z

src/test/java/com/github/checkstyle/regression/configuration/ConfigGeneratorTest.java

+    }
+
+    @After
+    public void tearDown() throws Exception {


DiffParserTest had this at the top of the class while in here you put in the middle.
Let's be consistent and always have After/Before at the top of the class so it is the first thing users see when reading this file.

Luolc · 2017-07-05T03:00:15Z

@rnveach Sorry I commited the changes but forgot to push. Updated are pushed now.

Luolc force-pushed the issue20 branch 3 times, most recently from 4d2cc44 to 6044d29 Compare July 1, 2017 17:37

rnveach requested changes Jul 2, 2017

View reviewed changes

Luolc force-pushed the issue20 branch 2 times, most recently from 12f42a9 to 8532b26 Compare July 3, 2017 16:42

Luolc force-pushed the issue20 branch from 8532b26 to 62752ef Compare July 3, 2017 16:58

Luolc mentioned this pull request Jul 3, 2017

ConfigGenerator: generate module parent node dynamically instead of hardcoding #47

Open

Luolc force-pushed the issue20 branch from 62752ef to 75c5740 Compare July 3, 2017 17:41

rnveach requested changes Jul 3, 2017

View reviewed changes

Luolc force-pushed the issue20 branch 2 times, most recently from e637302 to 16c4dbd Compare July 4, 2017 04:04

rnveach requested changes Jul 4, 2017

View reviewed changes

Issue checkstyle#20: Configuration Component

22a277a

Luolc force-pushed the issue20 branch from 16c4dbd to 22a277a Compare July 5, 2017 02:58

rnveach approved these changes Jul 5, 2017

View reviewed changes

rnveach merged commit befb36a into checkstyle:master Jul 5, 2017

Luolc deleted the issue20 branch July 5, 2017 14:55

Issue #20: Configuration Component #43

Issue #20: Configuration Component #43

Conversation

Luolc commented Jun 29, 2017

rnveach left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rnveach Jul 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rnveach commented Jul 3, 2017

Luolc commented Jul 3, 2017

rnveach commented Jul 3, 2017

Luolc commented Jul 3, 2017

rnveach left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rnveach Jul 3, 2017 • edited Loading

Choose a reason for hiding this comment

Luolc Jul 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Luolc Jul 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Luolc commented Jul 5, 2017

rnveach Jul 3, 2017 •

edited

Loading

rnveach Jul 3, 2017 •

edited

Loading

Luolc Jul 4, 2017 •

edited

Loading

Luolc Jul 4, 2017 •

edited

Loading