:technologist: JsonTransformer enhancement to support import scenarios by mesketh · Pull Request #653 · datafaker-net/datafaker

mesketh · 2023-01-28T00:51:59Z

JsonTransformer didn't have 4 indentation hence, the whitespace noise.
Added JSONAssert to assist with json assertions. Did not update other test cases to use this.

what-the-diff · 2023-01-28T00:52:07Z

Added a new dependency to the pom.xml file
Modified JsonTransformer class by adding support for formatting as array and object, added builder pattern
Added a new test case for outputting JSON as an array.
Fixed the formatting of some tests to be more readable and consistent with other code in this project (e.g., using static imports).
Updated dependencies, including adding org/skyscreamer/jsonassert:1.6 dependency so that we can use it in our new test case above (Bump junit from 4.12 to 4.13.1 #1)

codecov-commenter · 2023-01-28T01:00:16Z

Codecov Report

Merging #653 (8e7afca) into main (a1e090e) will decrease coverage by 0.15%.
The diff coverage is 92.52%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@             Coverage Diff              @@
##               main     #653      +/-   ##
============================================
- Coverage     92.79%   92.64%   -0.15%     
  Complexity     2623     2623              
============================================
  Files           281      281              
  Lines          5396     5413      +17     
  Branches        589      590       +1     
============================================
+ Hits           5007     5015       +8     
- Misses          239      245       +6     
- Partials        150      153       +3

Impacted Files	Coverage Δ
...net/datafaker/transformations/JsonTransformer.java	`90.99% <92.52%> (-1.65%)`	⬇️
...ain/java/net/datafaker/providers/base/Barcode.java	`85.71% <0.00%> (-4.77%)`	⬇️
.../main/java/net/datafaker/service/FakerContext.java	`83.58% <0.00%> (-4.48%)`	⬇️
...ker/idnumbers/pt/br/IdNumberGeneratorPtBrUtil.java	`92.59% <0.00%> (-3.71%)`	⬇️
...in/java/net/datafaker/providers/base/Locality.java	`62.96% <0.00%> (-1.86%)`	⬇️
...in/java/net/datafaker/providers/base/Computer.java	`100.00% <0.00%> (ø)`
.../datafaker/transformations/sql/SqlTransformer.java	`88.19% <0.00%> (+0.73%)`	⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

snuyanzin · 2023-01-28T21:01:51Z

+            sb.append(value);
+        } else if (value instanceof Collection) {
+            sb.append(generate((Collection) value));
+        } else if (value.getClass().isArray()) {
+            sb.append(generate(Arrays.asList((Object[]) value)));
+        } else {
+            String val = String.valueOf(value);
+            boolean toWrap = !val.startsWith("#{json");
+            if (toWrap) {
+                sb.append("\"");
+            }
+            for (char c : String.valueOf(value).toCharArray()) {
+                sb.append(ESCAPING_MAP.getOrDefault(c, c + ""));
+            }
+            if (toWrap) {
+                sb.append("\"");
+            }
+        }
+    }
+
+    private static String generate(Collection<Object> collection) {
+        StringBuilder sb = new StringBuilder();
+        sb.append("[");
+        int i = 0;
+        for (Object value : collection) {
+            if (i > 0) {
+                sb.append(", ");
+            }
+            i++;
+            value2String(value, sb);
+        }
+        sb.append("]");
+        return sb.toString();
+    }
+
+
+    private static Map<Character, String> createEscapeMap() {
+        final Map<Character, String> map = new HashMap<>();
+        map.put('\\', "\\\\");
+        map.put('\"', "\\\"");
+        map.put('\b', "\\b");


Why do we need to touch these lines within this PR?

editorconfig says 4 indent for java - this class was 2.

it should be in a separate commit since it has nothing to do with mentioned improvement.
Moreover splitting it in different commits will simplify git blame surfing in future

snuyanzin · 2023-01-28T21:02:02Z

+        map.put('\f', "\\f");
+        map.put('\n', "\\n");
+        map.put('\r', "\\r");
+        map.put('\t', "\\t");
+        map.put('/', "\\/");
+        map.put('\u0000', "\\u0000");
+        map.put('\u0001', "\\u0001");
+        map.put('\u0002', "\\u0002");
+        map.put('\u0003', "\\u0003");
+        map.put('\u0004', "\\u0004");
+        map.put('\u0005', "\\u0005");
+        map.put('\u0006', "\\u0006");
+        map.put('\u0007', "\\u0007");
+        // map.put('\u0008', "\\u0008");
+        // covered by map.put('\b', "\\b");
+        // map.put('\u0009', "\\u0009");
+        // covered by map.put('\t', "\\t");
+        // map.put((char) 10, "\\u000A");
+        // covered by map.put('\n', "\\n");
+        map.put('\u000B', "\\u000B");
+        // map.put('\u000C', "\\u000C");
+        // covered by map.put('\f', "\\f");
+        // map.put((char) 13, "\\u000D");
+        // covered by map.put('\r', "\\r");
+        map.put('\u000E', "\\u000E");
+        map.put('\u000F', "\\u000F");
+        map.put('\u0010', "\\u0010");
+        map.put('\u0011', "\\u0011");
+        map.put('\u0012', "\\u0012");
+        map.put('\u0013', "\\u0013");
+        map.put('\u0014', "\\u0014");
+        map.put('\u0015', "\\u0015");
+        map.put('\u0016', "\\u0016");
+        map.put('\u0017', "\\u0017");
+        map.put('\u0018', "\\u0018");
+        map.put('\u0019', "\\u0019");
+        map.put('\u001A', "\\u001A");
+        map.put('\u001B', "\\u001B");
+        map.put('\u001C', "\\u001C");
+        map.put('\u001D', "\\u001D");
+        map.put('\u001E', "\\u001E");
+        map.put('\u001F', "\\u001F");
+        return Collections.unmodifiableMap(map);
+    }


Why do we need to touch these lines within this PR?

snuyanzin · 2023-01-28T21:03:04Z

+        return Collections.unmodifiableMap(map);
+    }
+    private static char[] delimitedBy(boolean formatAsArray) {
+        return formatAsArray ? new char[]{'[',']'} : new char[] {'{','}'};


It seems it creates a new array each call which is not good for performance.
There are cases when we generate millions of rows and this will slow this down

snuyanzin · 2023-01-28T21:03:45Z

+    private static void value2String(Object value, StringBuilder sb) {
+        if (value == null) {
+            sb.append("null");
+        } else if (value instanceof Integer
+            || value instanceof Long
+            || value instanceof Short
+            || value instanceof BigInteger
+            || value instanceof Boolean
+            || (value instanceof Double
            && BigDecimal.valueOf((Double) value).remainder(BigDecimal.ONE).doubleValue() == 0)
-        || (value instanceof BigDecimal
+            || (value instanceof BigDecimal


Why do we need to touch these lines within this PR?

ditto - editorconfig violation/drift - this realigns (pardon the pun).

snuyanzin · 2023-01-28T21:04:06Z

+
+    private static final Map<Character, String> ESCAPING_MAP = createEscapeMap();
+
+    private boolean formatAsArray;


it should be final

snuyanzin · 2023-01-28T21:04:28Z

+    @Override
+    public String apply(IN input, Schema<IN, ?> schema) {
+        Field<?, ?>[] fields = schema.getFields();
+        StringBuilder sb = new StringBuilder();
+        sb.append("{");
+        for (int i = 0; i < fields.length; i++) {
+            value2String((fields[i].getName()), sb);
+            sb.append(": ");
+            if (fields[i] instanceof CompositeField) {
+                sb.append(apply(input, (CompositeField) fields[i], i));
+            } else {
+                value2String(((SimpleField) fields[i]).transform(input), sb);
+            }
+            if (i < fields.length - 1) {
+                sb.append(", ");
+            }
+        }
+        sb.append("}");
+        return sb.toString();
    }


Why do we need to touch these lines within this PR?

editorconfig violation? formatting only. Not sure why this file seemed to have drifted from others - i did check the history and it's always had 2 indent. 🤷

it's better not to mix checkstyle changes with PR related in same commit

I use intellij. It bundles an EditorConfig plugin and applies formatting changes automatically.

snuyanzin · 2023-01-28T21:05:39Z

+        JsonTransformerBuilder<Object> jsonTransformerBuilder = new JsonTransformerBuilder<>();
+        JsonTransformer<Object> transformer = jsonTransformerBuilder.build();
        String json = transformer.generate(schema, 2);
-


do we need this change?

snuyanzin · 2023-01-29T09:29:00Z

+        map.put('\u001F', "\\u001F");
+        return Collections.unmodifiableMap(map);
+    }
+    private static char[] delimitedBy(boolean formatAsArray) {


I tend to think that the name is confusing since it is not clear what delimiter is implied...
E.g. , is also a delimiter between array/object elements however it does not depend on this method result.

May be something like isArray or something related to this

Well it's all based on the context of use so you don't get confused if you're using it correctly and follow the examples set. Delimiting something is just setting the boundaries so it does apply here IMO but, I want to get this change in so I'm not arguing! I could add a comment but, I'm detecting a precedent of not commenting privates.

🧑‍💻 JsonTransformer enhancement to support different import scenarios.

5a22059

snuyanzin reviewed Jan 28, 2023

View reviewed changes

Addressing review comments.

9d48158

snuyanzin reviewed Jan 29, 2023

View reviewed changes

Clarifying logic applying top level json delimiters json as wrappers.

8e7afca

snuyanzin merged commit 9cb702b into datafaker-net:main Jan 29, 2023

mesketh deleted the jsontransformer-enhancement branch February 4, 2023 04:41


		private static final Map<Character, String> ESCAPING_MAP = createEscapeMap();

		private boolean formatAsArray;

Conversation

mesketh commented Jan 28, 2023

Uh oh!

what-the-diff Bot commented Jan 28, 2023

Uh oh!

codecov-commenter commented Jan 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

snuyanzin Jan 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mesketh Jan 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov-commenter commented Jan 28, 2023 •

edited

Loading

snuyanzin Jan 29, 2023 •

edited

Loading

mesketh Jan 29, 2023 •

edited

Loading