Issue #19: Module Component #33

Luolc · 2017-06-17T15:12:24Z

This PR contains many things. Some changes and discussions may be better in other issue/PR. Free feel to point them out and create new issues for them.

I introduced Immutables. It is a apt tool to generate immutable java object with boilerplate methods, builder, and serialization/deserialization supports etc. Our POJOs might become huge in the future(i.e. ModuleInfo would contains many fields). So I think it is a good choice to introduce such library. FYI: libraries with similar function: AutoValue, FreeBuilder
Currently I created the hardcode module info in json format. https://github.com/Luolc/regression-tool/blob/ee1fa4767cd6237f93683685d9e9c3a71a3ffced/src/main/resources/checkstyle_modules.json. I used the hardcode map in PackageObjectFactory and write a code snippet in PackageObjectFactoryTest to generate it. Contains the following info: name, parent, packageName, hierarchies, interfaces. But hierarchies, interfaces haven't been used in code yet.
I used Gson to handle the hardcode info. In fact, the json is in a file so that when we dynamically generate the json in the future, the production code won't change a lot.
I changed the order of dependencies in pom.xml, splitting main and test dependencies. Maybe it is better to do that in a separate PR.
pom.xml was changed a bit to exclude the generated files from coverage check.

rnveach

Maybe it is better to do that in a separate PR.

Separate commit is fine, can be minor.

If you have any concerns/questions, feel free to ask.

rnveach · 2017-06-19T11:42:34Z

src/main/resources/checkstyle_modules.json

@@ -0,0 +1,2340 @@
+[
+  {
+    "name": "Checker",


When I said hardcoded list, I meant like PackageObjectFactory did at https://github.com/checkstyle/checkstyle/blob/b024af7dbc3f53863e17147692100381f4165073/src/main/java/com/puppycrawl/tools/checkstyle/PackageObjectFactory.java#L338 , as we are not sure yet if it will be a file with XML/JSON or something else.
It can stay for now.

@Luolc See #37 .
Lets remove this file from main and use it only in test for our UTs.
It will only be used in #37 .

rnveach · 2017-06-19T11:44:28Z

config/import-control.xml

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


Please add space after this item, and between each subpackage.

rnveach · 2017-06-19T11:49:38Z

config/import-control.xml

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


org.immutables. All this just to have it create the class for us?
I am not a fan of auto-generated classes as you can't see the code for it. We also lost code coverage for it, so we can't tell if a field is ever used or if a method definition covers all branches.

Why do we need it for simple POJOs when we don't do this for the other POJOs in git component?
I will get back to you on this item.

@Luolc I asked romani his opinion on this, and he isn't a fan of "magic" builders either.
Since this isn't the main repository, we could keep this here if we really wanted to.

My only concern right now is code coverage as ModuleInfo isn't showing any or requiring any, even for hasCheckerParent which is a branch because of the equality test.

I believe there would be many fields in ModuleInfo in the future.
If we use constructor to pass these parameters, it would be ugly. 《Effective Java》 said it is better to use builder style if constructor has more than three parameters.
Another annoying thing is the equals method. We would have a lot of equals asserting in UTs. It is hard to appease codecov if we write this manually. Supposed there are 5 fields then the equals method would have 32 branches(2^5). That's horrible. If we write a equals method in UTs, yeah that could be a workaround, be a little bit strange. And it is also bad that when we add or change some fields, then we have to update the utility equals in UTs.

So I think it is not bad if an annotation processing tool could generate these for us. We could then focus on more important things but not writing builders, equals etc.

Supposed there are 5 fields then the equals method would have 32 branches(2^5). That's horrible.

I assume we got around this in main project using equalsverifier dependency.

So I think it is not bad if an annotation processing tool could generate these for us

Let's keep org.immutables than, but how about we require no custom methods in the interface and require them to be in utility classes so we can keep code coverage on them? I dislike the idea of having code we can't be sure we are covering.

Keeping them as abstract classes seems to keep them requiring coverage, so lets stick with this.
This is done.

rnveach · 2017-06-19T11:55:25Z

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

+ * Contains the logic of regression config generation.
+ * @author LuoLiangchen
+ */
+public final class ConfigGenerator {


IMO, this is a bad name for a class in the module component, especially since we will have another component called configuration. This will easily confuse people and they question why we don't have ConfigGenerator in configuration component.
We need a more suitable name for this and generateConfigNodes.

rnveach · 2017-06-19T11:55:40Z

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

+     * @param changes the changes source
+     * @return the regression config nodes generated from the given changes
+     */
+    public static List<ConfigNode> generateConfigNodes(List<GitChange> changes) {


I am not sure if this component should return ConfigNode.
When we transverse through the changes, modules and tests will be in separate files, but we have to identify them together and build multiple modules from the data in the 2.

Will ConfigNode cause a lot of extra searching if we leave it in this format? How will you maintain list of properties and values to cycle through?
Would a map of module names and a custom class of their retrieved data be better ? Would create the actual configuration hierarchy in configuration component.

I think it depends on the duty scope of configuration. I was thinking that configuration would only get direct data describing the XML.(module name, property name, value etc.) If we are going to pass more data to configuration, then how detailed it would be?

I guess I misunderstood some duties of these two component.

The only duty of configuration is build the configuration file. There are no specifics on the type of data it should retrieve.
Someone has to build the configuration, and someone has to generate the list of different modules to build based on properties and such. It's not a concern who does which, but do we need an intermediate data type before the direct data describing the XML?

I was thinking that configuration would only get direct data describing the XML.

My reasoning against this is "what if we have to build a new component between module and configuration". Can this new component deal with the design structure of the final configuration? Would it just want to see a single module as a whole?

We can hold this off for now, but I am just worried about if this structure will be good for #5 as we need to create multiple module configurations for each set of properties.

Which is why I think the answer to How will you maintain list of properties and values to cycle through? is an important question.

rnveach · 2017-06-19T12:13:36Z

src/main/java/com/github/checkstyle/regression/module/ModuleUtils.java

+ */
+public final class ModuleUtils {
+    /** The compiled regex pattern of the path of Java main source files. */
+    private static final Pattern MAIN_SOURCE_PATTERN = Pattern.compile("src/main/java/(.+)\\.java");


Will windows require \? If so, we may need [\\/] like our suppressions.
I will look into this and get back.

ConfigGeneratorTest fails when I change all file paths from / to \\.
I used another utility I wrote with jgit to see how it returns files and folders. DiffEntry's paths are returned with / in Windows.

So it seems this won't be an issue and can be ignored for now.

rnveach · 2017-06-19T12:15:59Z

src/main/java/com/github/checkstyle/regression/module/ModuleUtils.java

+            }
+        }
+        catch (IOException ex) {
+            throw new IllegalStateException("Failed when loaing hardcode information", ex);


hardcode information => hardcoded checkstyle module information
Best to be as specific as possible.

rnveach · 2017-06-19T12:23:13Z

src/main/java/com/github/checkstyle/regression/util/GsonProvider.java

+    static {
+        final GsonBuilder gsonBuilder = new GsonBuilder();
+        for (TypeAdapterFactory factory : ServiceLoader.load(TypeAdapterFactory.class)) {
+            gsonBuilder.registerTypeAdapterFactory(factory);


What are these 2 lines doing?
Can and should this be embedded with the other file stuff?

This is for registering the GsonTypeAdapter generated by immutables. It is sort of initialization of Gson.
FYI: https://immutables.github.io/json.html#type-adapter-registration

As discussed below, lets keep all gson stuff together in one class.

rnveach · 2017-06-19T12:31:37Z

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

+    }
+
+    @Test
+    public void testGenerateConfigNodes() {


Split this into 2 tests. Invalid and valid changes. (valid = recognize as checkstyle module)
Add a new invalid location inside JAVA_MAIN_SOURCE_PREFIX and inside src\main\java but both not recognized as a module. This is an example of a checkstyle utility class or a brand new module that we didn't recognize.

I split the current UT into two. Not very clear about "new invalid" mean. And what does "inside" mean?
Added UT:

@Test public void testGenerateConfigNodesForInvalidChanges() { final List<GitChange> changes = new LinkedList<>(); changes.add(new GitChange( JAVA_MAIN_SOURCE_PREFIX + "PackageObjectFactory.java")); changes.add(new GitChange("foo/A.java")); final List<ConfigNode> configNodes = ConfigGenerator.generateConfigNodes(changes); assertEquals(0, configNodes.size()); }

a new invalid location inside JAVA_MAIN_SOURCE_PREFIX

I guess changes.add(new GitChange(JAVA_MAIN_SOURCE_PREFIX + "PackageObjectFactory.java")); is this situation.

inside src\main\java

Do you mean smth like src/main/java/Foo.java?

"new invalid" mean

I just meant a new file/folder that you hadn't used before.

"inside" mean

A file inside that folder location.

I guess ...JAVA_MAIN_SOURCE_PREFIX + "PackageObjectFactory.java"

Yes.

Do you mean smth like src/main/java/Foo.java?

Yes.

rnveach · 2017-06-19T12:35:52Z

src/test/java/com/github/checkstyle/regression/module/ModuleUtilsTest.java

+        final InputStream is = spy(ModuleUtils.class.getClassLoader()
+                .getResourceAsStream("checkstyle_modules.json"));
+        doThrow(IOException.class).when(is).close();
+        try {


empty line before try.

rnveach · 2017-06-19T13:20:56Z

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

+import com.github.checkstyle.regression.data.ImmutableConfigNode;
+
+public class ConfigGeneratorTest {
+    private static final String BASE_PACKAGE = "com.puppycrawl.tools.checkstyle";


Field is never used.

rnveach · 2017-06-21T12:50:21Z

config/import-control.xml

+    </subpackage>
+
+    <subpackage name="util">
+        <allow pkg="com.google.gson"/>


I'm not sure what happened to my original post on this as I can't find it anymore.

I am fine with util package, but it seems to me we are breaking component design by using component specific com.google.gson outside of module component, which is the only component that will use it right?
Should this be a util inside module?

It also seems like ModuleUtils is trying to do too much. It does json reading and other various things.
I think we should have one utility for JSON, including code in GsonProvider, and have another class that is what utility methods module needs.

@Luolc Please see this.

Lets move all gson code to new component in #37 .
Either this component will now receive the collection of ModuleExtractInfos, or a utility class will and keep it as global information.

rnveach · 2017-06-23T11:59:02Z

@Luolc ping
Its been 2 days without any acitivity here. Is everything going ok?

Luolc · 2017-06-23T19:15:31Z

@rnveach

I added the middleware ModuleUpdate. What's your opinion of its current contents? I am not very confident of it and need your suggestion.
I haven't handled the location of gson util class and splitting gson related code apart from ModuleUtils yet. Maybe focus on them sooner.

rnveach · 2017-06-23T20:59:42Z

ModuleUpdate

I don't think this class can be fully immutable. How are you going to combine different changes in separate GitChange? Will you recreate the entire class to add a new change?
The point of the middleware is that it will be in flux while we parse all the git changes. We have to be able to add everything we need to this one class. I am not expecting GitChange to go to next component. module will do everything with it now.

Here is what I recommend:

Rename ModuleInfo to ModuleExtractInfo and ModuleUpdate to ModuleInfo and ModuleUpdateCollector to ModuleCollector.
ModuleInfo should be a non-immutable class and contain: name (module), module extract, list of unique property names and values (ignore properties for now).
Rename generateModuleUpdates to generate.
Inside parse:
a) create map of module name and ModuleInfo.
b) for each change,
i) If file is module, add module to map if doesn't exist
ii) If file is test module, add module to map if doesn't exist (eventually expanding this to grab properties)
c) return changes as list.

When we go to next component, it will take ModuleInfo and start creating the configuration from it.
Eventually, if no properties are found or not all, we will generate our own (some how).

Does this make sense?

Luolc · 2017-06-24T06:05:29Z

How are you going to combine different changes in separate GitChange?

I update the definition of ModuleInfo. I am using a ModifiableModuleInfo now.

If file is test module, add module to map if doesn't exist

What if in a PR there is only one test file changed? In this case we don't need regression. We have many issues that only need to change test scope without touching production code.

IMO, we should go through the production code changes first, adding some modules to the map(direct change now, and gain related modules from utility changes in the future). Then, go through the test changes. If the map contains the corresponding module already, then process the test change, otherwise just ignore it.

rnveach · 2017-06-24T12:31:46Z

IMO, we should go through the production code changes first, adding some modules to the map(direct change now, and gain related modules from utility changes in the future). Then, go through the test changes. If the map contains the corresponding module already, then process the test change, otherwise just ignore it.

Agreed.

Luolc · 2017-06-24T16:44:45Z

Rename ModuleInfo to ModuleExtractInfo and ModuleUpdate to ModuleInfo and ModuleUpdateCollector to ModuleCollector.
ModuleInfo should be a non-immutable class and contain: name (module), module extract, list of unique property names and values (ignore properties for now).
Rename generateModuleUpdates to generate.
Inside parse:
a) create map of module name and ModuleInfo.
b) for each change,
i) If file is module, add module to map if doesn't exist
ii) If file is test module, add module to map if doesn't exist (eventually expanding this to grab properties)
c) return changes as list.

Updated according to the suggestions. Please have a check.

rnveach · 2017-06-25T23:58:23Z

@Luolc Everything is looking good.

Just to summarize:

Lets move all gson code to new component in #37 . You can make a new PR now with just this code if you wish but I still prefer it in one file.
Either this component will now receive the collection of ModuleExtractInfos, or a utility class will and keep it as global information.

Lets remove checkstyle_modules.json from main and use it only in test for our UTs.
It will only be used in #37 .

Remove commented code. If it is needed for future items, keep them as a copy somewhere.

Coverage can be less than 100% for now to get this merged faster.

Split dependency re-ordering into minor commit.

Luolc · 2017-06-26T02:10:02Z

Is this component only for json related code and named as json?

Don't have questions on others

rnveach · 2017-06-26T02:40:03Z

Is this component only for json related code and named as json?

Answered in issue #37 .

Luolc · 2017-06-26T17:02:07Z

Split dependency re-ordering into minor commit

Split to #38

Then changes about adding objects to data are split to #39 . It is blocked by #38 , we have to finish #38 and then rebase and go on.

Lets move all gson code to new component in #37

In #40 , also, blocked by #38 and #39

Other changes are also done and pushed, but need these PRs above are merged and rebase them.

rnveach · 2017-06-27T17:00:50Z

Please rebase to remove conflicts. Only changes for main component should remain.
Gson data outside of data package shouldn't be needed.
Update code coverage based on any new tests added in this PR.

Luolc · 2017-06-28T17:36:40Z

Rebased and pushed. I am adding the assert messages in test now.

Edit: I uncommented the codes about test and utility classes in ModuleUtils and add the codecov settings of this file to 57% 33% in pom. We should have a issue for this if this PR is merged.

rnveach · 2017-06-29T02:06:26Z

src/main/java/com/github/checkstyle/regression/module/ModuleUtils.java

+     * Checks whether the corresponding file of a change may be considered as
+     * a Java main source file.
+     * @param change change to check
+     * @return true if the corresponding file of a change may be considered as


duplicating main description in return tag is bad javadoc design. You are not giving reader anything new to read.
Can you try to fix these and others by making each unique and different information and not just a rewording?

I add some info in each main description. Refers to the similar utility methods in ModuleReflectionUtils in checkstyle project.

rnveach · 2017-06-29T02:10:14Z

src/main/java/com/github/checkstyle/regression/module/ModuleUtils.java

+     */
+    public static boolean isCheckstyleModuleTest(GitChange change) {
+        final boolean returnValue;
+        if (JAVA_SOURCE_PARTTEN.matcher(change.getPath()).find()) {


This is true for test and main. It should only be accepting test. Make a isJavaTestSource and use that.

rnveach · 2017-06-29T02:11:30Z

src/main/java/com/github/checkstyle/regression/module/ModuleUtils.java

+     * Sets the map of full qualified name to module extract info with the given map.
+     * @param map the given map
+     */
+    public static void setNameToModuleExtratInfo(Map<String, ModuleExtractInfo> map) {


Extrat => Extract

rnveach · 2017-06-29T02:18:16Z

src/main/java/com/github/checkstyle/regression/module/ModuleUtils.java

+     * @param change the change to convert
+     * @return the full name of corresponding checkstyle module
+     */
+    public static String convertModuleTestChangeToModuleFullName(GitChange change) {


There is no test that change is in test and no test that testName ends with Test like isCheckstyleModuleTest.

These 2 convert methods (convertModuleTestChangeToModuleFullName and convertModuleChangeToExtractInfo) make me feel like you are trying to put too much into this class that may not be best as a utility method. They are so reliant on external factors on other tests done before this method is called.

Should we make convertJavaSourcePathToFullName public and have the main component do the work instead of these methods?

The main concern is NAME_TO_MODULE_EXTRACT_INFO map. It should be placed in ModuleUtils since isCheckstyleModule, isCheckstyleUtility, isCheckstyleModuleTest all need it.

convertModuleTestChangeToModuleFullName is OK to move out from ModuleUtils now. But convertModuleChangeToExtractInfo could not, since we need access NAME_TO_MODULE_EXTRACT_INFO to get the converted info. I don't think we should place NAME_TO_MODULE_EXTRACT_INFO somewhere else.

My opinion now is: keeping these two methods here, then adding a if-condition at the beginning of each, throwing a IllegalArgumentException if the parameter is not valid(not checkstyle module, or not checkstyle module test)

These methods just seem too specific to be an actual utility method. 1 is unused, so it could be one issue for why I can't see a good way to fix it right now.

Yes, the map should stay.
I was thinking not to move them out but having simplier methods like getModuleInfo (take full module name as string parameter) so you don't have to move it out. The only issue for the test method would be TEST_POSTFIX which I don't see how you can get around for now.

Otherwise, you can throw the exception, but the test one also needs to make sure it ends with Test.
Usage of this method should never throw the exception, so you have to be real careful in it's use. I might make an issue to look at it later when we implement more in the module component.

I was thinking not to move them out but having simplier methods like getModuleInfo (take full module name as string parameter)

I am fine with this.

Since actually we haven't use convertModuleTestChangeToModuleFullName now. How about just remove it now and decide it later when we have more codes of handling test files later. Maybe it would be clearer of how to handle this at that time.

Yes, just remove it than.

rnveach · 2017-06-29T02:19:41Z

src/main/java/com/github/checkstyle/regression/module/ModuleCollector.java

+     */
+    public static List<ModuleInfo> generate(List<GitChange> changes) {
+        final Map<String, ModifiableModuleInfo> moduleInfos = new LinkedHashMap<>();
+        for (GitChange change : changes) {


empty line before for.

rnveach · 2017-06-29T02:24:46Z

src/main/java/com/github/checkstyle/regression/module/ModuleUtils.java

+     * @param path the path of Java source file
+     * @return the corresponding full qualified name
+     */
+    private static String convertJavaSourcePathToFullName(String path) {


convertModuleChangeToExtractInfo and convertModuleTestChangeToModuleFullName don't really need GitChange but you leave convertJavaSourcePathToFullName as String even though everywhere that uses it has GitChange available.

Change this parameter to GitChange.

rnveach · 2017-06-29T02:28:40Z

src/main/java/com/github/checkstyle/regression/module/ModuleUtils.java

+    private static final String TEST_POSTFIX = "Test";
+
+    /** The map of full qualified name to module extract info. */
+    private static final Map<String, ModuleExtractInfo> NAME_TO_MODULE_EXTRAT_INFO =


EXTRAT => EXTRACT
Check for this misspelling everywhere.

rnveach

Final thing.

rnveach · 2017-06-29T10:08:42Z

src/main/java/com/github/checkstyle/regression/module/ModuleUtils.java

+     * @return true if the corresponding file of a change may be considered as
+     *      a Java test source file.
+     */
+    private static boolean isJavaTestSource(GitChange change) {


One last thing, please group isJava... methods together. Do the same for isCheckstyle... methods.

rnveach · 2017-06-29T12:03:30Z

Merged.

Luolc mentioned this pull request Jun 17, 2017

Module Extract File #29

Closed

rnveach requested changes Jun 19, 2017

View reviewed changes

Luolc force-pushed the issue19 branch 2 times, most recently from 5530428 to 607235b Compare June 21, 2017 12:39

rnveach reviewed Jun 21, 2017

View reviewed changes

Luolc force-pushed the issue19 branch from 607235b to d99b0dd Compare June 21, 2017 14:21

Luolc force-pushed the issue19 branch 2 times, most recently from 6c42f41 to 9c0b260 Compare June 23, 2017 19:08

Luolc force-pushed the issue19 branch from 9c0b260 to 6365459 Compare June 24, 2017 09:48

Luolc force-pushed the issue19 branch from 6365459 to 82164b5 Compare June 24, 2017 16:44

Luolc force-pushed the issue19 branch from 82164b5 to 583617c Compare June 26, 2017 16:01

This was referenced Jun 26, 2017

Issue #19: Add data object #39

Merged

Issue #37: json file processor #40

Merged

Luolc force-pushed the issue19 branch 2 times, most recently from fe701c4 to a26ba49 Compare June 28, 2017 17:34

Luolc force-pushed the issue19 branch from a26ba49 to bcb323f Compare June 28, 2017 17:46

rnveach requested changes Jun 29, 2017

View reviewed changes

Luolc force-pushed the issue19 branch 2 times, most recently from 57c2f7b to 5a0a1b1 Compare June 29, 2017 09:48

rnveach requested changes Jun 29, 2017

View reviewed changes

Luolc force-pushed the issue19 branch 4 times, most recently from b6ff965 to 0be132c Compare June 29, 2017 11:33

Issue checkstyle#19: Module Component

5c15b05

Luolc force-pushed the issue19 branch from 0be132c to 5c15b05 Compare June 29, 2017 11:33

rnveach approved these changes Jun 29, 2017

View reviewed changes

rnveach merged commit 4939fb0 into checkstyle:master Jun 29, 2017

Luolc deleted the issue19 branch June 29, 2017 18:48

Issue #19: Module Component #33

Issue #19: Module Component #33

Conversation

Luolc commented Jun 17, 2017 • edited Loading

rnveach left a comment

Choose a reason for hiding this comment

rnveach Jun 19, 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

rnveach Jun 19, 2017 • edited Loading

Choose a reason for hiding this comment

rnveach Jun 19, 2017 • edited Loading

Choose a reason for hiding this comment

Luolc Jun 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rnveach Jun 25, 2017 • edited Loading

Choose a reason for hiding this comment

rnveach Jun 19, 2017 • edited Loading

Choose a reason for hiding this comment

rnveach Jun 19, 2017 • edited Loading

Choose a reason for hiding this comment

Luolc Jun 21, 2017 • edited Loading

Choose a reason for hiding this comment

rnveach Jun 21, 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 Jun 19, 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

rnveach Jun 21, 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 Jun 21, 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 23, 2017

Luolc commented Jun 23, 2017 • edited Loading

rnveach commented Jun 23, 2017

Luolc commented Jun 24, 2017 • edited Loading

rnveach commented Jun 24, 2017

Luolc commented Jun 24, 2017

rnveach commented Jun 25, 2017 • edited Loading

Luolc commented Jun 26, 2017

rnveach commented Jun 26, 2017

Luolc commented Jun 26, 2017

rnveach commented Jun 27, 2017 • edited Loading

Luolc commented Jun 28, 2017 • edited Loading

rnveach Jun 29, 2017 • edited Loading

Choose a reason for hiding this comment

Luolc Jun 29, 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

rnveach Jun 29, 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

Luolc commented Jun 17, 2017 •

edited

Loading

rnveach Jun 19, 2017 •

edited

Loading

rnveach Jun 19, 2017 •

edited

Loading

rnveach Jun 19, 2017 •

edited

Loading

Luolc Jun 21, 2017 •

edited

Loading

rnveach Jun 25, 2017 •

edited

Loading

rnveach Jun 19, 2017 •

edited

Loading

rnveach Jun 19, 2017 •

edited

Loading

Luolc Jun 21, 2017 •

edited

Loading

rnveach Jun 21, 2017 •

edited

Loading

rnveach Jun 19, 2017 •

edited

Loading

rnveach Jun 21, 2017 •

edited

Loading

rnveach Jun 21, 2017 •

edited

Loading

Luolc commented Jun 23, 2017 •

edited

Loading

Luolc commented Jun 24, 2017 •

edited

Loading

rnveach commented Jun 25, 2017 •

edited

Loading

rnveach commented Jun 27, 2017 •

edited

Loading

Luolc commented Jun 28, 2017 •

edited

Loading

rnveach Jun 29, 2017 •

edited

Loading

Luolc Jun 29, 2017 •

edited

Loading

rnveach Jun 29, 2017 •

edited

Loading