Refactor processor to make extension easier by bndbsh · Pull Request #53 · danielnorberg/auto-matter

bndbsh · 2017-06-01T17:27:56Z

This delegates most of the field-specific processing to FieldProcessor,
which can be implemented to customize handling of different types.
Collections are handled by CollectionProcessor, optionals by
OptionalProcessor. Everything else is handled by DefaultProcessor.

A data-driven approach was taken, with FieldProcessor taking the
descriptor and field as input and outputting Javapoet specs or
statements. However, in this patch CollectionProcessor is dependent on
AutoMatterProcessor for the implementation of singular.

There is also some duplication and inefficiencies in the processor as a
result of this refactor. Since doing away with them would change text
order in some places and thus require edits to most of the test files,
I prefer leaving that work to a followup commit to reduce review burden.

Other notes:

It's not clear where common code should live. For now most of it is
in default methods on FieldProcessor, but I'm open to suggestions.
Due to the above use of default, source level 1.8 is now required
I wasn't sure how to organize the classes, so I've left them all at
top level. It might become clearer once common code is centralized
and other processors are added.
Not sure if this is feasible, but it could be interesting to allow
users to supply their own processors via some annotation, or to
register them somehow with AutoMatter. This would allow extensions to
live in separate packages.

This delegates most of the field-specific processing to `FieldProcessor`, which can be implemented to customize handling of different types. Collections are handled by `CollectionProcessor`, optionals by `OptionalProcessor`. Everything else is handled by `DefaultProcessor`. A data-driven approach was taken, with `FieldProcessor` taking the descriptor and field as input and outputting Javapoet specs or statements. However, in this patch `CollectionProcessor` is dependent on `AutoMatterProcessor` for the implementation of `singular`. There is also some duplication and inefficiencies in the processor as a result of this refactor. Since doing away with them would change text order in some places and thus require edits to most of the test files, I prefer leaving that work to a followup commit to reduce review burden. Other notes: * It's not clear where common code should live. For now most of it is in default methods on `FieldProcessor`, but I'm open to suggestions. * Due to the above use of default, source level 1.8 is now required * I wasn't sure how to organize the classes, so I've left them all at top level. It might become clearer once common code is centralized and other processors are added. * Not sure if this is feasible, but it could be interesting to allow users to supply their own processors via some annotation, or to register them somehow with AutoMatter. This would allow extensions to live in separate packages.

danielnorberg · 2017-06-05T06:20:39Z

processor/src/main/java/io/norberg/automatter/processor/AutoMatterProcessor.java

  }

+  private FieldProcessor processorForField(Descriptor d, ExecutableElement field) throws AutoMatterProcessorException {
+    String prefix = fieldType(d, field).toString().split("<")[0];


Use guava Splitter instead of compiling a regex for every invocation.

danielnorberg · 2017-06-05T06:23:21Z

processor/src/main/java/io/norberg/automatter/processor/AutoMatterProcessor.java

  private Messager messager;
  private Types types;
+  private Map<String, FieldProcessor> processors;
+  private DefaultProcessor defaultProcessor = new DefaultProcessor();


danielnorberg · 2017-06-05T06:24:18Z

processor/src/main/java/io/norberg/automatter/processor/AutoMatterProcessor.java

  private Elements elements;
  private Messager messager;
  private Types types;
+  private Map<String, FieldProcessor> processors;


Would it make sense for this to be a Map<Class, FieldProcessor> instead?

danielnorberg · 2017-06-05T06:24:40Z

processor/src/main/java/io/norberg/automatter/processor/AutoMatterProcessor.java

+    OptionalProcessor optionalProcessor = new OptionalProcessor();
+
+    processors = ImmutableMap.of(
+      "java.util.Map", collectionProcessor,


Would be nice if we could avoid these handcrafted strings.

danielnorberg · 2017-06-05T06:27:27Z

processor/src/main/java/io/norberg/automatter/processor/AutoMatterProcessor.java

  }

-  private TypeName builderType(final Descriptor d) {
+  public static TypeName builderType(final Descriptor d) {


Move to a separate class and avoid circular imports?

danielnorberg · 2017-06-05T06:28:18Z

processor/src/main/java/io/norberg/automatter/processor/BuildStatements.java

@@ -0,0 +1,11 @@
+package io.norberg.automatter.processor;
+
+public class BuildStatements {


package-private

danielnorberg · 2017-06-05T06:28:37Z

processor/src/main/java/io/norberg/automatter/processor/BuildStatements.java

+package io.norberg.automatter.processor;
+
+public class BuildStatements {
+  Iterable<Statement> statements;


danielnorberg · 2017-06-05T06:30:32Z

processor/src/main/java/io/norberg/automatter/processor/AutoMatterProcessor.java

-      } else {
-        parameters.add(fieldName(field));
+      BuildStatements buildStatements = processorForField(d, field).build(d, field);
+      for (Statement s: buildStatements.statements) {


Whitespace around :

danielnorberg · 2017-06-05T06:32:28Z

processor/src/main/java/io/norberg/automatter/processor/CollectionProcessor.java

+import static javax.lang.model.element.Modifier.PUBLIC;
+import static javax.lang.model.type.TypeKind.DECLARED;
+
+public class CollectionProcessor implements FieldProcessor {


package-private

danielnorberg · 2017-06-05T06:34:20Z

processor/src/main/java/io/norberg/automatter/processor/CollectionProcessor.java

+public class CollectionProcessor implements FieldProcessor {
+  private final AutoMatterProcessor processor;
+
+  public CollectionProcessor(AutoMatterProcessor processor) {


Can we factor things to avoid this circular dependency of passing in the AutoMatterProcessor into field processors?

danielnorberg · 2017-06-05T06:45:38Z

processor/src/main/java/io/norberg/automatter/processor/FieldProcessor.java

+  BuildStatements build(Descriptor d, ExecutableElement field) throws AutoMatterProcessorException;
+  Iterable<Statement> valueConstructor(Descriptor d, ExecutableElement field) throws AutoMatterProcessorException;
+
+  default TypeName fieldType(final Descriptor d, final ExecutableElement field) throws AutoMatterProcessorException {


I guess these methods aren't actually part of the FieldProcessor interface. I.e., FieldProcessor users aren't expected to call them.

How about grouping and moving them to separate classes based on functionality? E.g. put the field introspection methods into e.g. a Fields class etc.

danielnorberg · 2017-06-05T06:45:53Z

processor/src/main/java/io/norberg/automatter/processor/OptionalProcessor.java

+import static io.norberg.automatter.processor.AutoMatterProcessor.builderType;
+import static javax.lang.model.element.Modifier.PUBLIC;
+
+public class OptionalProcessor extends DefaultProcessor {


package-private

danielnorberg · 2017-06-05T06:45:59Z

processor/src/main/java/io/norberg/automatter/processor/Statement.java

@@ -0,0 +1,11 @@
+package io.norberg.automatter.processor;
+
+public class Statement {


package-private

danielnorberg · 2017-06-05T06:46:04Z

processor/src/main/java/io/norberg/automatter/processor/Statement.java

+package io.norberg.automatter.processor;
+
+public class Statement {
+  String statement;


danielnorberg · 2017-06-05T07:05:23Z

This is a much-needed refactoring. Kudos!

Looks great overall, just a few small comments.

Eliminate circular imports and dependencies.
Everything should be package-private or private.
Leave everything at top level for now.

Allowing extensions in separate packages at sounds great. AutoMatterProcessor can use j.u.ServiceLoader to discover extensions and extensions can use AutoService to register themselves. Should be straightforward but let's do it in a separate PR.

bndbsh · 2017-07-12T16:10:35Z

Re: using Map<Class, FieldProcessor> instead of string, that would require having the necessary classes in the classpath wouldn't it? Maybe we can do ClassName instead?

danielnorberg reviewed Jun 5, 2017

View reviewed changes

Move common methods around to avoid circular dependencies

8015512

bndbsh force-pushed the refactor branch from ffdc693 to 8015512 Compare July 12, 2017 16:07

		@@ -0,0 +1,11 @@
		package io.norberg.automatter.processor;

		public class BuildStatements {

		@@ -0,0 +1,11 @@
		package io.norberg.automatter.processor;

		public class Statement {

Conversation

bndbsh commented Jun 1, 2017

Uh oh!

danielnorberg Jun 5, 2017 • 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!

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!

danielnorberg commented Jun 5, 2017

Uh oh!

bndbsh commented Jul 12, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

danielnorberg Jun 5, 2017 •

edited

Loading