Issue #37: json file processor #40

Luolc · 2017-06-26T16:54:49Z

Issue #37

Taken from #33 , this is blocked by #38 and #39

rnveach · 2017-06-26T19:15:41Z

src/main/java/com/github/checkstyle/regression/extract/ExtractInfoProcessor.java

+                    new InputStreamReader(stream, Charset.forName("UTF-8"));
+
+            try {
+                modules = GSON.fromJson(reader, new TypeToken<List<ModuleExtractInfo>>() {


Please move new TypeToken to a static final field.

rnveach · 2017-06-26T19:18:29Z

src/main/java/com/github/checkstyle/regression/extract/ExtractInfoProcessor.java

+     * @return the full qualified name to module extract info map
+     */
+    public static Map<String, ModuleExtractInfo> getNameToModuleExtractInfoFromInputStream(
+            InputStream stream) {


InputStream is fine, but you take that and than wrap it around InputStreamReader for UTF 8 support.
Shouldn't class that creates the stream define what charset the file should support and not this method?
I think we should either remove InputStreamReader and stick with InputStream or change this parameter to InputStreamReader.

change this parameter to InputStreamReader

Done.

rnveach · 2017-06-26T19:22:02Z

src/test/java/com/github/checkstyle/regression/extract/ExtractInfoProcessorTest.java

+                .getResourceAsStream("checkstyle_modules.json");
+        final Map<String, ModuleExtractInfo> map =
+                ExtractInfoProcessor.getNameToModuleExtractInfoFromInputStream(is);
+        assertEquals(165, map.size());


Trim test json file to only include items needed for specific testing.
We don't need full copy of checkstyle repo for basic testing.
If we find something we need later in it, we will expand.

OK. And, IMO, it is better to create another json file only for ExtractInfoProcessorTest, with a name like checkstyle_modules_sample.json(and maybe in resources/.../extract).

rnveach

Please rebase on latest master. Than I can more closely review what is only for this PR.

rnveach · 2017-06-27T16:56:34Z

config/import-control.xml

@@ -7,6 +7,15 @@
    <allow pkg="java.io"/>
    <allow pkg="java.util"/>
    <allow pkg="com.github.checkstyle.regression.data"/>
+    <allow pkg="org.immutables"/>


This and other similar items should go away with rebase.

rnveach · 2017-06-27T16:58:43Z

src/test/java/com/github/checkstyle/regression/extract/ExtractInfoProcessorTest.java

+        final InputStreamReader reader = new InputStreamReader(is, Charset.forName("UTF-8"));
+        final Map<String, ModuleExtractInfo> map =
+                ExtractInfoProcessor.getNameToModuleExtractInfoFromInputStream(reader);
+        assertEquals(165, map.size());


Was listed as outdated but is still not done.

it is better to create another json file only for ExtractInfoProcessorTest, with a name like checkstyle_modules_sample.json

I am fine with 1 or multiple json files. If we do multiple, they should be trimmed down to bare bones for test.

Emm... now I decide to keep this json file name currently. And if we find it is necessary to split the json file when we are doing main code of module component, we do the split then.

Done.

Luolc · 2017-06-27T17:37:40Z

Done: Rebased and json trimmed.

rnveach · 2017-06-27T19:41:47Z

config/import-control.xml

+        <allow pkg="com.google.gson"/>
+        <allow class="com.sun.tools.javac.util.ServiceLoader"/>
+        <allow class="java.lang.reflect.Type"/>
+        <allow class="java.nio.charset.Charset"/>


This shouldn't be needed since it is not imported in main code.
Please remove and confirm the others are needed.

rnveach · 2017-06-27T19:44:25Z

src/main/java/com/github/checkstyle/regression/extract/package-info.java

+////////////////////////////////////////////////////////////////////////////////
+
+/**
+ * Contains checkstyle maven command executing codes and extract file processor.


executing codes?
Change to Contains execution of checkstyle maven extract process and...

rnveach · 2017-06-27T19:47:35Z

src/test/java/com/github/checkstyle/regression/extract/ExtractInfoProcessorTest.java

+                .packageName(BASE_PACKAGE + ".checks.coding")
+                .parent("TreeWalker")
+                .build();
+        assertEquals(2, map.size());


We started requiring assertion messages in main repo by PMD.
This project doesn't have the newly updated PMD, but I may add it soon.

Please make sure all new assertions in PRs have message. It can be very simple phrase.
Check out main project for any examples.

rnveach · 2017-06-27T19:49:46Z

src/main/java/com/github/checkstyle/regression/extract/ExtractInfoProcessor.java

+     * @param reader the given reader
+     * @return the full qualified name to module extract info map
+     */
+    public static Map<String, ModuleExtractInfo> getNameToModuleExtractInfoFromReader(


getNameToModuleExtractInfo
This makes it sound like we are passing to the method name as a parameter and it is giving us ModuleExtractInfo. We aren't giving it any parameters.
Please rename to something like getModuleExtractInfoFromReader or such.

rnveach · 2017-06-27T19:52:10Z

src/main/java/com/github/checkstyle/regression/extract/ExtractInfoProcessor.java

+            }
+        }
+        catch (IOException ex) {
+            throw new IllegalStateException(


Throwing as a runtime exception is fine, but we will never be able to catch it in Main if we wish to add any more information to it. Please confirm this is your intent.

The only way to go into catch block is reader.close() throwing an exception. I think it is rare. Instead of this, GSON.fromJson is more likely to throwing exception sometimes (wrong format, etc.). Json exception is not caught here so we could use them in main if needed.

Json exception is not caught here so we could use them in main

Method has no throws Exception declaration so then all throwable exceptions, if any, are runtime.
We can't do anything about GSON but if we know an error is possible, we should not use runtime exceptions.
We can ignore this for now.

rnveach

Everything else looks fine and I will merge when done.

rnveach · 2017-06-28T12:11:29Z

src/main/java/com/github/checkstyle/regression/extract/ExtractInfoProcessor.java

+    }
+
+    /**
+     * Gets the full qualified name to module extract info map from the given reader.


Please remember to update Javadoc if you change method. This is the 3rd time now.
Gets the full qualified name to module extract info map => Gets the module extract info map

I did check this javadoc and I thought a "key to value map" description is clearer and kept it.

My building is on a unexpected power cut god...
I can only use GitHub on my phone and there is no edit function on it...

Maybe map of full qualified name to module extract info is better? It refers the javadoc of the hardcode map in PackageObjectFactory. I am not able to write the link to it on my phone...

I really think a key to value description is necessary. If no description of the key in neither javadoc nor method name, it's confused what the key is when we use the method elsewhere

@Luolc The issue is name to module extract info which is what method was named and we did away with it at #40 (comment) . This is specifically the text I want removed. Anything else is fine.

a "key to value map" description is clearer

How about this: Gets the module extract info map from the given reader. Map key is the fully qualified module name.

It's good. :D Updated.

rnveach · 2017-06-28T17:13:28Z

Merged.

@Luolc As you do only pieces of issue, please keep main issue updated with what is left to do in new post.

Luolc mentioned this pull request Jun 26, 2017

Issue #19: Module Component #33

Merged

rnveach requested changes Jun 26, 2017

View reviewed changes

Luolc force-pushed the issue37-part1 branch from 19e50c8 to 431bf3c Compare June 27, 2017 16:26

rnveach requested changes Jun 27, 2017

View reviewed changes

Luolc force-pushed the issue37-part1 branch 3 times, most recently from 222ebb4 to fcf0f9c Compare June 27, 2017 17:36

rnveach requested changes Jun 27, 2017

View reviewed changes

Luolc force-pushed the issue37-part1 branch from fcf0f9c to 10c56e8 Compare June 28, 2017 09:18

rnveach requested changes Jun 28, 2017

View reviewed changes

Issue checkstyle#37: json file processor

aafce55

Luolc force-pushed the issue37-part1 branch from 10c56e8 to aafce55 Compare June 28, 2017 17:07

rnveach approved these changes Jun 28, 2017

View reviewed changes

rnveach merged commit 567d5b7 into checkstyle:master Jun 28, 2017

Luolc deleted the issue37-part1 branch June 28, 2017 17:12

Luolc mentioned this pull request Jun 28, 2017

Extract Component - Generate Module Extract list #37

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue #37: json file processor #40

Issue #37: json file processor #40

Luolc commented Jun 26, 2017 •

edited by rnveach

Loading

rnveach Jun 26, 2017

Luolc Jun 27, 2017

rnveach Jun 26, 2017

Luolc Jun 27, 2017

rnveach Jun 26, 2017

Luolc Jun 27, 2017

rnveach left a comment •

edited

Loading

rnveach Jun 27, 2017

rnveach Jun 27, 2017

Luolc Jun 27, 2017 •

edited

Loading

Luolc commented Jun 27, 2017

rnveach Jun 27, 2017

Luolc Jun 28, 2017

rnveach Jun 27, 2017

Luolc Jun 28, 2017

rnveach Jun 27, 2017

Luolc Jun 28, 2017

rnveach Jun 27, 2017 •

edited

Loading

Luolc Jun 28, 2017

rnveach Jun 27, 2017

Luolc Jun 28, 2017

rnveach Jun 28, 2017

rnveach left a comment

rnveach Jun 28, 2017

Luolc Jun 28, 2017

Luolc Jun 28, 2017

Luolc Jun 28, 2017

rnveach Jun 28, 2017 •

edited

Loading

rnveach Jun 28, 2017

Luolc Jun 28, 2017

rnveach commented Jun 28, 2017

Issue #37: json file processor #40

Issue #37: json file processor #40

Conversation

Luolc commented Jun 26, 2017 • edited by rnveach 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

rnveach left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Luolc Jun 27, 2017 • edited Loading

Choose a reason for hiding this comment

Luolc commented Jun 27, 2017

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 Jun 27, 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

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 Jun 28, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rnveach commented Jun 28, 2017

Luolc commented Jun 26, 2017 •

edited by rnveach

Loading

rnveach left a comment •

edited

Loading

Luolc Jun 27, 2017 •

edited

Loading

rnveach Jun 27, 2017 •

edited

Loading

rnveach Jun 28, 2017 •

edited

Loading