Redone work on adding IndexUnit (Tests and Travis job passed) #2

Bohdankm22 · 2017-06-17T01:24:35Z

created IndexUnit class that contains only field List
changed java.jj file to read the stubfiles and generate the IndexUnit object
created JavaParser.class parseIndex and parseResourceIndex that are the copy of the parse and parseResource respectively with the difference in the return type (IndexUnit vs CompilationUnit).

Maven test and Travis job passed
https://travis-ci.org/Bohdankm22/javaparser/builds/243882845

- created IndexUnit class that contains only field List<CompilationUnit> - changed java.jj file to read the stubfiles and generate the IndexUnit object - created JavaParser.class parseIndex and parseResourceIndex that are the copy of the parse and parseResource respectively with the difference in the return type (IndexUnit vs CompilationUnit).

mernst

This design is much better! I have a few comments.

mernst · 2017-06-19T12:40:37Z

javaparser-core/src/main/java/com/github/javaparser/JavaParser.java

@@ -42,7 +43,6 @@
 import java.io.*;
 import java.nio.charset.Charset;
 import java.nio.file.Path;
-import java.util.TreeSet;


I would not remove this line (even though it wasn't used in the original file).
The reason is that doing so increases the size of the diff and increases the possibility for merge conflicts in the future.

mernst · 2017-06-19T12:48:20Z

javaparser-core/src/main/java/com/github/javaparser/JavaParser.java

@@ -295,6 +295,148 @@ public static CompilationUnit parse(String code) {
        return simplifiedParse(COMPILATION_UNIT, provider(code));
    }

+    public static IndexUnit parseIndexUnit(final InputStream in, Charset encoding) {


Please write a Javadoc comment for every method and field.

mernst · 2017-06-19T12:49:49Z

javaparser-core/src/main/java/com/github/javaparser/JavaParser.java

+     * Note: Uses UTF-8 encoding
+     *
+     * @param path path to a resource containing Java source code. As resource is accessed through a class loader, a
+     * leading "/" is not allowed in pathToResource


Minor: add period at end of sentence.

mernst · 2017-06-19T12:50:13Z

javaparser-core/src/main/java/com/github/javaparser/JavaParser.java

+
+    /**
+     * Parses the Java code contained in a resource and returns a
+     * {@link IndexUnit} that represents it.<br>


The trailing <br> is not necessary.

mernst · 2017-06-19T12:50:27Z

javaparser-core/src/main/java/com/github/javaparser/JavaParser.java

+     * {@link IndexUnit} that represents it.<br>
+     *
+     * @param path path to a resource containing Java source code. As resource is accessed through a class loader, a
+     * leading "/" is not allowed in pathToResource


Missing period. (The same comment applies a few other places below.)

mernst · 2017-06-19T13:16:06Z

javaparser-core/src/main/javacc/java.jj

@@ -672,6 +672,74 @@ TOKEN :
 /*
 * Program structuring syntax follows.
 */
+IndexUnit IndexUnit():


Since this is an addition and not part of the Java language, there should be a comment. The comment should also point at a user-visible description of what may appear in an IndexUnit.

mernst · 2017-06-19T13:16:58Z

javaparser-core/src/main/javacc/java.jj

@@ -672,6 +672,74 @@ TOKEN :
 /*
 * Program structuring syntax follows.
 */
+IndexUnit IndexUnit():


What is the reason for the name IndexUnit? I would have expected a name that uses the word Stub, which is what is used in the Checker Framework documentation.

mernst · 2017-06-19T13:19:26Z

javaparser-core/src/main/javacc/java.jj

+	NodeList<CompilationUnit> compilationUnits = emptyList();
+	CompilationUnit cu;
+
+	/* Parsing the compilation unit itself */


I don't understand this comment.

Maybe you mean that the above two variables are the representation of the object being parsed, and the remaining variables are temporary variables that will be used during parsing. Whether or not my guess is right, please clarify.

mernst · 2017-06-19T13:24:33Z

javaparser-core/src/main/javacc/java.jj

+    )
+
+    [ LOOKAHEAD(PackageDeclaration()) pakage = PackageDeclaration() ]
+        (


Why does this repeat the identical text (but with different indentation!) as "A A*" rather than using the "A+" syntax? That is, can you use Kleene Plus here instead of Kleene Star? If not, explain why not.

mernst · 2017-06-19T14:29:23Z

javaparser-core/src/main/javacc/java.jj

+                )
+            )
+            {
+                 cu = new CompilationUnit(range(token_source.getHomeToken(), token()), pakage, imports, types, module);


This seems to require a correct, complete compilation unit. Is it your plan in the future to relax this restriction so that various parts (such as method bodies) can be omitted? That's fine, but the scope of this particular pull request wasn't clear from the description.

Yes, that is right. I will work on relaxing restrictions later.

Added throwing the RuntimeException instead of returning the null value.

… class that reflects the stub files.

- Added comments of the fields declarations and stub files reading - Added using of A+ syntax instead of A A* syntax

… in ObservableProperties and ModifierVisitor

…throw a RuntimeException with a message that the method is not implemented

…hanges done in javaparser project.

…ber of changes done in javaparser project.

mernst

Looks good overall. Thanks! Please address my comments and then merge.

mernst · 2017-06-23T17:56:42Z

javaparser-core/src/main/java/com/github/javaparser/JavaParser.java

@@ -43,7 +44,6 @@
 import java.nio.charset.Charset;
 import java.nio.file.Path;
 import java.util.TreeSet;
-


Please avoid whitespace changes. (Even if they are good, they can lead to merge conflicts.)

mernst · 2017-06-23T18:03:16Z

javaparser-core/src/main/java/com/github/javaparser/ast/StubUnit.java

+/**
+ * <p>
+ * This class represents a stub file. The stub file is a Java file, but with the optionally omitted
+ * information that is not relevant to pluggable type-checking; this makes the stub file smaller


Add a cross-reference to the full definition of a stub file. (You may need to update that definition as the project proceeds.)

mernst · 2017-06-23T18:03:50Z

javaparser-core/src/main/java/com/github/javaparser/ast/StubUnit.java

+
+/**
+ * <p>
+ * This class represents a stub file. The stub file is a Java file, but with the optionally omitted


A stub file isn't just a Java file with some information omitted. It's a concatenation of multiple Java files.
Without this information, "the list of compilation units" below is confusing.

mernst · 2017-06-23T18:07:27Z

javaparser-core/src/main/java/com/github/javaparser/ast/StubUnit.java

+public class StubUnit extends Node {
+
+    /**
+     * The field represents a list of compilations units.


The field doesn't represent a list of compilation units; it is a list of compilation units. Make the comment give information beyond the type if possible.

(Typo: "compilations units" should be "compilation unit".)

mernst · 2017-06-23T18:10:12Z

javaparser-core/src/main/java/com/github/javaparser/ast/StubUnit.java

+    private StubUnit.Storage storage;
+
+    /**
+     * The constructor that takes the tokenRange and just pass it to the super method {@link Node}.


Internal implementation details don't belong in Javadoc. It's better to say what the TokenRange is or represents or how it is used, if possible.

Also, Javadoc should say what the method does, not what it is. Rather than, "the constructor that...", write something like "Constructs a StubUnit from a list of tokens".

mernst · 2017-06-23T18:21:09Z

javaparser-core/src/main/javacc/java.jj

+	* Represents the particular compilation unit components in each iteration of
+	* reading compilation units from the stub file
+	*/
+	PackageDeclaration pakage = null;


There is still inconsistent use of tabs vs. spaces in this file.
(The lines that start with * aren't properly indented either.)

mernst · 2017-06-23T18:22:41Z

javaparser-core/src/main/javacc/java.jj

+	CompilationUnit cu;
+
+	/*
+	* Represents the particular compilation unit components in each iteration of


What are the "particular" ones?

mernst · 2017-06-23T18:23:33Z

javaparser-core/src/main/javacc/java.jj

+	( LOOKAHEAD(2) ";" )*
+	(
+	    /*
+	    * Reading the particular compilation unit in the stub file


What do you mean by "particular" compilation units? Do you just mean the ones in the file?
Minor (but aids readability): end a sentence with punctuation rather than making the reader guess where sentences end and start.

mernst · 2017-06-23T18:23:52Z

javaparser-core/src/main/javacc/java.jj

+	(
+	    /*
+	    * Reading the particular compilation unit in the stub file
+	    * Number of compilation units could be 1+


Not "could be" but "is".

mernst · 2017-06-23T18:28:50Z

javaparser-core/src/main/javacc/java.jj

+                 cu = new CompilationUnit(range(token_source.getHomeToken(), token()), pakage, imports, types, module);
+                 compilationUnits.add(cu);
+            }
+        )


You have made a design choice to remove the * that appears at this location in CompilationUnit. That affects how a file is split up: you split it up into the largest possible number of CompilationUnits as opposed to the smallest possible number. That's fine, but perhaps worth commenting about.

- Moving reading package declaration (at the begin of the stub file is required); - Reading additional package declarations later in the stub file (not required by the stub file format); - Reading imports (not required by the stub file format); - Constructors and method bodies could be omitted for the stub files; - Number of compilation units could be 1+ according to the stub file structure; - Added comments in java.jj file. Maven test is successful. Tested on the flow.astub file (need to move package declaration to the beginning of the file).

- Reformatted changes done in java.jj file to be consistent; - Added fixes to the comments

Bohdankm22 · 2017-06-24T01:47:09Z

@mernst I have finished work on your suggestions. However, I have also added one important commit with changes in the way compiler is going to read the stub files.
734ebb8
I tested parsing stub files on the flow.astub (moved package declaration to the beginning of the file) and got a good result.

mernst · 2017-06-24T12:21:55Z

What is your plan for merging this pull request? Once a pull request has been reviewed and approved, it's generally best to merge it. Otherwise, if you continue to make changes, then the whole pull request has to be re-reviewed.

mernst · 2017-06-24T12:22:59Z

I tried to fix some English errors myself, but I don't seem to have permission to push to your repository. Could you please grant me that permission?

Bohdankm22 · 2017-06-24T17:14:23Z

@mernst, got it.
I have finished work on this pull request. I am not going to add anything else here. Once you approve it I will merge it.
Thank you!

Matozoid issue 912 modifier node

* Add Grammar for Javadoc * Integrate into main parser We are currently able to parse the Javadoc comment, but cannot construct an object from it as of yet. Everything else is finished. Co-authored-by: yiiju <yijuhung0129@gmail.com>

Updating with changes from javaparser repository

Bohdankm22 added 2 commits June 16, 2017 21:14

Fixing the imports list. Removing the blank line

fd6d975

mernst requested changes Jun 19, 2017

View reviewed changes

Bohdankm22 added 8 commits June 19, 2017 21:52

Undo the unnecessary changes done previously.

d73884d

Added throwing the RuntimeException instead of returning the null value.

Renaming the IndexUnit to the StubUnit which is better represents the…

5055c1c

… class that reflects the stub files.

Changes in java.jj file:

48177d1

- Added comments of the fields declarations and stub files reading - Added using of A+ syntax instead of A A* syntax

Undo unnecessary changes.

2473aa1

Undo unnecassery changes (removing whitespaces and shuffling imports)…

ecebf66

… in ObservableProperties and ModifierVisitor

Changed not implemented visit methods with StubParser as argument to …

2a159ff

…throw a RuntimeException with a message that the method is not implemented

- Undid changes done with shuffling imports to reduce the number of c…

a4c669d

…hanges done in javaparser project.

- Removed the constructor that was added previously to reduce the num…

b8df1be

…ber of changes done in javaparser project.

mernst approved these changes Jun 23, 2017

View reviewed changes

Bohdankm22 added 3 commits June 23, 2017 16:57

Making the documentation to the StubUnit class more clear.

2061278

Changes to java.jj file:

5c4bc4b

- Reformatted changes done in java.jj file to be consistent; - Added fixes to the comments

Bohdankm22 and others added 2 commits June 23, 2017 21:48

Fixed a mistake in comment.

5893b91

English fix

d2838e0

Fixed indentation in comments in java.jj file.

5c47b78

Bohdankm22 merged commit 7fa2c15 into master Jun 25, 2017

Bohdankm22 deleted the index_new branch June 25, 2017 16:41

mernst referenced this pull request in mernst/stubparser Jan 7, 2019

Merge pull request #2 from ftomassetti/matozoid-issue_912_modifier_node

8c3b948

Matozoid issue 912 modifier node

mernst referenced this pull request in mernst/stubparser Nov 11, 2020

Merge pull request #2 from javaparser/master

06d202a

Updating with changes from javaparser repository

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Redone work on adding IndexUnit (Tests and Travis job passed) #2

Redone work on adding IndexUnit (Tests and Travis job passed) #2

Bohdankm22 commented Jun 17, 2017

mernst left a comment

mernst Jun 19, 2017

mernst Jun 19, 2017

mernst Jun 19, 2017

mernst Jun 19, 2017

mernst Jun 19, 2017

mernst Jun 19, 2017

mernst Jun 19, 2017

mernst Jun 19, 2017

mernst Jun 19, 2017

mernst Jun 19, 2017

Bohdankm22 Jun 21, 2017

mernst left a comment

mernst Jun 23, 2017

mernst Jun 23, 2017

mernst Jun 23, 2017

mernst Jun 23, 2017

mernst Jun 23, 2017

mernst Jun 23, 2017

mernst Jun 23, 2017

mernst Jun 23, 2017

mernst Jun 23, 2017

mernst Jun 23, 2017

Bohdankm22 commented Jun 24, 2017

mernst commented Jun 24, 2017

mernst commented Jun 24, 2017

Bohdankm22 commented Jun 24, 2017

Redone work on adding IndexUnit (Tests and Travis job passed) #2

Redone work on adding IndexUnit (Tests and Travis job passed) #2

Conversation

Bohdankm22 commented Jun 17, 2017

mernst 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

mernst 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

Bohdankm22 commented Jun 24, 2017

mernst commented Jun 24, 2017

mernst commented Jun 24, 2017

Bohdankm22 commented Jun 24, 2017