Instagram / ig-json-parser

Fast JSON parser for java projects
https://instagram-engineering.com/fast-auto-generated-streaming-json-parsing-for-android-ab8e7be21033
MIT License
1.32k stars 124 forks source link

Add annotation imports #52

Closed jingibus closed 6 years ago

jingibus commented 6 years ago

Another small feature addition while i'm ramped up on this codebase.

This adds two parameters to @JsonType: imports, and calleeImports.

imports is straightforward: anything in imports goes into the imports section in generated code. This helps clean up custom code specified in @JsonField:

@JsonType(imports = "java.util.Formatter")
public class ImportsUUT {

    @JsonField(fieldName = "string_field",
            valueExtractFormatter =
                "new Formatter().format(\":%%s\", ${parser_object}.getText()).toString()")
    public String mStringField;
}

calleeImports is the same thing, but for custom code specified on @JsonType. The usage is clearer than the mechanism, so here's a usage example:

@JsonType(
    valueExtractFormatter = "CalleeImportsCompanionUUT__JsonHelper.parseFromJson(${parser_object})",
    calleeImports = {
        "com.instagram.common.json.annotation.processor.parent.CalleeImportsCompanionUUT__JsonHelper"
    })
public class CalleeImportsUUT {
  @JsonField(fieldName = "string_field")
  public String mString;
}

These imports are needed whenever the valueExtractFormatter is used, so they are added in generated classes that refer to this class.

kangzhang commented 6 years ago

I like this API, it will make code easier to read. A couple thoughts:

1) maybe formatterImports v.s. helperImports/imports (wondering how we can make it easier to understand)? 2) not sure if it's possible in Annotation, but the javapoet API which references class directly(as opposed to String), feels more refactor friendly. E.g. is something like following possible at all?

JsonType(
  formatterImports = [DateTime.class];
)
jingibus commented 6 years ago
  1. I like s/calleeImports/formatterImports/. I can clarify the docs with that name, too, I think.
  2. :+1: I thought String would make static imports possible, but it doesn't.
jingibus commented 6 years ago

Here's my revised documentation:

   * Additional imports to include in generated code for this class. These imports are visible
   * from formatter code on {@link JsonField}.
   * They will not be visible from formatters on {@link JsonType}.
   *
   * <p>These imports will be unconditionally added to this class's generated JsonHelper.</p>
   */
  String [] imports() default {};

  /**
   * Additional import to include in generated code that refers to this class. These imports are
   * visible from formatter code on {@link JsonType}. They will not be visible from formatters
   * on {@link JsonField}.
   *
   */
  String [] formatterImports() default {};

Writing this documentation, the thought occurred to me that the names fieldImports and typeImports might be clearer.

jingibus commented 6 years ago

Unfortunately, the Class<?> type will not work. Classes are not available at annotation processing time. (writing ClassName.class.getName() in the annotation won't work, either)

kangzhang commented 6 years ago

gotcha, thanks for digging into it!

perhaps we could add some sanity check for the string to make sure it's valid import statement, wondering if the compile error will be harder to read otherwise.

i can merge as it is, but looks like there are merge conflicts. could you rebase this one to master?

jingibus commented 6 years ago

JavaWriter already sanity checks those strings for us. Not perfect, but this will get errored out at annotation processing time. Example error message:

12:32:37.072 [ERROR] [system.err] warning: ERROR: annotation exception: java.lang.IllegalArgumentException: com   .instagram.common.json.annotation.processor.parent.TypeFormatterImportsCompanionUUT__JsonHelper cause: java.lang.IllegalArgumentException: com   .instagram.common.json.annotation.processor.parent.TypeFormatterImportsCompanionUUT__JsonHelper

...[giant obscure stack trace from JsonParserClassData]...
kangzhang commented 6 years ago

Looks great, thanks for all unit tests!