Fredy Wijaya has posted comments on this change. ( http://gerrit.cloudera.org:8080/12079 )
Change subject: IMPALA-7968, Part 1: JSON serialization framework ...................................................................... Patch Set 4: (5 comments) http://gerrit.cloudera.org:8080/#/c/12079/3/fe/src/main/java/org/apache/impala/common/serialize/ObjectSerializer.java File fe/src/main/java/org/apache/impala/common/serialize/ObjectSerializer.java: http://gerrit.cloudera.org:8080/#/c/12079/3/fe/src/main/java/org/apache/impala/common/serialize/ObjectSerializer.java@83 PS3, Line 83: objectList shouldn't this be stringList? http://gerrit.cloudera.org:8080/#/c/12079/3/fe/src/main/java/org/apache/impala/common/serialize/ObjectSerializer.java@88 PS3, Line 88: objectList should this be scalarList? http://gerrit.cloudera.org:8080/#/c/12079/3/fe/src/main/java/org/apache/impala/common/serialize/ToJsonOptions.java File fe/src/main/java/org/apache/impala/common/serialize/ToJsonOptions.java: http://gerrit.cloudera.org:8080/#/c/12079/3/fe/src/main/java/org/apache/impala/common/serialize/ToJsonOptions.java@34 PS3, Line 34: public ToJsonOptions showSource(boolean flag) : { showSource_ = flag; return this; } Sorry if I wasn't clear. I meant to use the standard style. public ToJsonOptions showSource(boolean flag) { showSource_ = flag; return this; } http://gerrit.cloudera.org:8080/#/c/12079/2/fe/src/test/java/org/apache/impala/common/serialize/TreeFormatterTest.java File fe/src/test/java/org/apache/impala/common/serialize/TreeFormatterTest.java: http://gerrit.cloudera.org:8080/#/c/12079/2/fe/src/test/java/org/apache/impala/common/serialize/TreeFormatterTest.java@39 PS2, Line 39: : protected abstract void build(); : : protected void verify(String expected) { : builder_.close(); : assertEquals(expected, strWriter_.toString()); : } > The style is common in tests to avoid accidental reuse of common variables What do you think about this? @FunctionalInterface private interface Verifier { void verify(String expected); } private static Verifier testBuilder(Consumer<TreeBuilder> consumer) { StringWriter strWriter = new StringWriter(); TreeBuilder builder = new TreeBuilder(new PrintWriter(strWriter)); consumer.accept(builder); return (expected) -> { builder.close(); assertEquals(expected, strWriter.toString()); }; } public static Verifier testFormatter(ToJsonOptions options, Consumer<JsonTreeFormatter> consumer) { JsonTreeFormatter fmt = new JsonTreeFormatter(options); fmt.build(); consumer.accept(fmt); return (expected) -> { assertEquals(expected, fmt.toString()); }; } public static Verifier testFormatter(Consumer<JsonTreeFormatter> consumer) { return testFormatter(ToJsonOptions.full(), consumer); } And we can use it like this: testBuilder(builder -> { assertEquals(1, builder.root()); }).verify("{\n}\n"); testFormatter(fmt -> { ObjectSerializer os = fmt.root(); os.field("bool", true); }).verify("{\n bool: true\n}\n"); IMO lambda is generally more readable for creating a DSL (and generates better code because of invokedynamic, although I don't think it matters for this unit test) than using anonymous inner class. http://gerrit.cloudera.org:8080/#/c/12079/2/fe/src/test/java/org/apache/impala/common/serialize/TreeFormatterTest.java@65 PS2, Line 65: > Would love to do it that way if Java had something like a "HERE" doc. But, I'm too pampered with IntelliJ that can nicely preserve the formatting for String multi-line :) Anyway, I'm okay with this. -- To view, visit http://gerrit.cloudera.org:8080/12079 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie3101c2708bf6cf4bec61af83a5db9b6d70ddd9c Gerrit-Change-Number: 12079 Gerrit-PatchSet: 4 Gerrit-Owner: Paul Rogers <[email protected]> Gerrit-Reviewer: Fredy Wijaya <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Paul Rogers <[email protected]> Gerrit-Reviewer: Philip Zeyliger <[email protected]> Gerrit-Comment-Date: Sat, 15 Dec 2018 04:15:15 +0000 Gerrit-HasComments: Yes
