Till Westmann has posted comments on this change. Change subject: TweetParser Extension ......................................................................
Patch Set 4: (10 comments) With the exception of the change in Lexer.java only easy-to-address coding style comments :) The change in Lexer.java looks like the line counting is broken today. Is that the case? Or is the another reason for the modification? https://asterix-gerrit.ics.uci.edu/#/c/1002/4/asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/input/record/reader/twitter/TwitterPullRecordReader.java File asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/input/record/reader/twitter/TwitterPullRecordReader.java: Line 31: import twitter4j.*; Please avoid * includes. I think that the eclipse code format profile at https://cwiki.apache.org/confluence/download/attachments/61322291/AsterixCodeFormatProfile.xml might already do this, but it might also be that it doesn't ... https://asterix-gerrit.ics.uci.edu/#/c/1002/4/asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/input/record/reader/twitter/TwitterPushRecordReader.java File asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/input/record/reader/twitter/TwitterPushRecordReader.java: Line 30: import twitter4j.*; Please avoid * includes. https://asterix-gerrit.ics.uci.edu/#/c/1002/4/asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/library/java/JObjectAccessors.java File asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/library/java/JObjectAccessors.java: Line 73: import org.apache.asterix.om.base.*; Please avoid * includes. Line 78: import org.apache.asterix.om.types.*; Please avoid * includes. Line 576: public static IAType ATypeMachine(ATypeTag typeTag){ Very similar code already exists in TypeTagUtil.getBuiltinTypeByTag in a more compact form. Could you reuse that (or follow its style if reuse is not feasible)? https://asterix-gerrit.ics.uci.edu/#/c/1002/4/asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/parser/TweetParser.java File asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/parser/TweetParser.java: Line 44: import static org.apache.asterix.om.types.ATypeTag.*; Please avoid * includes. Line 78: if(fieldType.getTypeTag() == BuiltinType.ASTRING.getTypeTag()){ Could we 'switch' here? Line 82: else if (fieldType.getTypeTag() == INT64){ We usually have the 'else' on the same line as the '}' (as you've also done a few lines below) - we 're to use the if-then-else here. Using the code format profile, should do that automatically. Line 239: } catch (Exception e) { Can we be more specific in the exception(s) that we catch here? https://asterix-gerrit.ics.uci.edu/#/c/1002/4/asterixdb/asterix-maven-plugins/lexer-generator-maven-plugin/src/main/resources/Lexer.java File asterixdb/asterix-maven-plugins/lexer-generator-maven-plugin/src/main/resources/Lexer.java: Line 88: // line++; Why is this needed? -- To view, visit https://asterix-gerrit.ics.uci.edu/1002 To unsubscribe, visit https://asterix-gerrit.ics.uci.edu/settings Gerrit-MessageType: comment Gerrit-Change-Id: I7021e7b779de05b9ec999a8d5f8464fb0ab413c0 Gerrit-PatchSet: 4 Gerrit-Project: asterixdb Gerrit-Branch: master Gerrit-Owner: Xikui Wang <[email protected]> Gerrit-Reviewer: Jenkins <[email protected]> Gerrit-Reviewer: Till Westmann <[email protected]> Gerrit-Reviewer: abdullah alamoudi <[email protected]> Gerrit-HasComments: Yes
