Use annotations for operation descriptions by SamCarlberg · Pull Request #851 · WPIRoboticsProjects/GRIP

SamCarlberg · 2017-05-05T20:32:15Z

Operations are created via the Guice Injector, making it much easier to use dependency injection in operations. Operations are also discovered at startup as long as they have an @Description annotation on the class

codecov-io · 2017-05-05T20:46:27Z

Codecov Report

Merging #851 into master will increase coverage by 0.25%.
The diff coverage is 74.5%.

@@             Coverage Diff              @@
##             master     #851      +/-   ##
============================================
+ Coverage     51.45%   51.71%   +0.25%     
- Complexity     1146     1158      +12     
============================================
  Files           247      247              
  Lines          7956     7812     -144     
  Branches        531      533       +2     
============================================
- Hits           4094     4040      -54     
+ Misses         3677     3592      -85     
+ Partials        185      180       -5

SamCarlberg · 2017-05-05T21:10:50Z

As an FYI the branch for CUDA support will be based on this for dependency injection

JLLeitschuh · 2017-05-05T21:17:35Z

core/src/main/java/edu/wpi/grip/core/OperationDescription.java

+  /**
+   * Creates an operation description from a {@link Description @Description} annotation on
+   * an operation subclass.
+   */


I think (and I'm totally okay with being wrong) but I think this code could go inside of Description because it is a fully qualified class and can have its own logic inside it.

No can do. Annotation interfaces can't have methods.

Okay, thats fine.

JLLeitschuh · 2017-05-05T21:23:26Z

core/src/test/java/edu/wpi/grip/core/operations/OperationsTest.java

  @Parameters(name = "{index}: Operation({0})")
  public static Collection<Object[]> data() {
-    EventBus eventBus = new EventBus();
+    System.out.println("data()");


Why did PMD not find this...

I don't think PMD runs on test files

Re-enable CompatibilityTest. Fix some backwards compatbility issues. Not really a fan of injecting the Injector into the Operations class. Need to see if there's a better solution

JLLeitschuh · 2017-05-09T15:27:49Z

core/src/main/java/edu/wpi/grip/core/operations/composite/FilterLinesOperation.java

-import edu.wpi.grip.core.util.Icon;

 import com.google.common.collect.ImmutableList;
+import com.google.inject.Inject;


Can you use javax.inject.Inject instead?

Prefer JSR-330's annotations and Provider interface.

https://github.com/google/guice/wiki/JSR330

Oh. That's going to be a lot of code changed because both are used everywhere. Should probably add a checkstyle rule for this in a different PR

JLLeitschuh · 2017-05-09T15:29:58Z

core/src/main/java/edu/wpi/grip/core/operations/network/Vector2D.java

 * {@link Size}s are converted into this.
 */
 @Immutable
+@PublishableProxy({Point.class, Size.class})


Do you want to use the constructor parameter to get the type?
Any reason why the type should be specified in the parameter and in the annotation?

We could iterate over all public constructors, but I like how this specifies exactly what's being proxied. It could also run into issues with publicly accessible helper constructors eg Vector2D(double, double)

Fine. This makes sense.

JLLeitschuh · 2017-05-09T15:33:59Z

core/src/test/java/edu/wpi/grip/core/operations/OperationsTest.java

+  // @Parameters is called before @BeforeClass, so we call it manually in a static initializer block
+  //  @BeforeClass
+  public static void setUpClass() {
+    System.out.println("setUpClass");


Remove this please.

This still needs to be addressed.

JLLeitschuh · 2017-05-09T15:34:15Z

core/src/test/java/edu/wpi/grip/core/operations/OperationsTest.java

+
+  @AfterClass
+  public static void tearDownClass() {
+    System.out.println("tearDownClass()");


Remove this please.

This still needs to be addressed.

JLLeitschuh · 2017-05-09T15:35:02Z

core/src/test/java/edu/wpi/grip/core/serialization/CompatibilityTest.java

-    OperationsFactory.create(eventBus).addOperations();
-    //CVOperations.addOperations(eventBus);
+    OperationsFactory.create(eventBus, injector).addOperations();
+    OperationsFactory.createCV(eventBus).addOperations();


Why was this commented out before?

JLLeitschuh · 2017-05-09T15:35:13Z

core/src/test/resources/edu/wpi/grip/projects/testALL.grip

      </grip:Input>
-      <grip:Input step="34" socket="2">
-        <value>0.0</value>
-      </grip:Input>


Why was this removed?

It was incompatible with the new version of the operation. I think the compatibility test was disabled for so long that it was never picked up

So somewhere along the way we had a breaking change that we never did a major version bump for?

Any update on this?

SamCarlberg · 2017-07-21T02:44:03Z

@JLLeitschuh want to revisit this?

JLLeitschuh · 2017-07-24T14:45:01Z

core/src/main/java/edu/wpi/grip/core/Description.java

+
+  /**
+   * The name of the icon to use to display the operation. If empty ({@code ""}), no icon will be
+   * shown. The icon should be located in {@code /edu/wpi/grip/ui/icons/}.


Is this the right use of this javadoc annotation? Does it render correctly?

Yeah, it's just a shortcut for <code>...</code>

JLLeitschuh · 2017-07-24T14:48:08Z

core/src/main/java/edu/wpi/grip/core/operations/network/ros/JavaToMessageConverter.java


  private abstract static class SimpleConverter<J, M extends Message> extends
-      JavaToMessageConverter<J, M> {
+                                                                      JavaToMessageConverter<J, M> {


Why is this whitespace diff here?

JLLeitschuh · 2017-07-24T14:49:25Z

core/src/main/java/edu/wpi/grip/core/serialization/StepConverter.java


    if (!operationMetaData.isPresent()) {
-      throw new ConversionException("Unknown operation: " + operationName);
+      throw new ConversionException(String.format("Unknown operation: %s. Known operations: %s",


Perhaps "Unknown operation: %s. did not match any known operations: %s"

JLLeitschuh · 2017-07-24T14:50:18Z

core/src/test/java/edu/wpi/grip/core/CoreSanityTest.java

        Main.class,
-        CoreCommandLineHelper.class
+        CoreCommandLineHelper.class,
+        OperationDescription.class


Why are you adding this class? Why not simply make it pass?

JLLeitschuh · 2017-07-24T14:57:01Z

core/src/main/java/edu/wpi/grip/core/operations/Operations.java

+             Injector injector,
             FileManager fileManager,
             InputSocket.Factory isf,
             OutputSocket.Factory osf) {


Does the OutputSocket.Factory still get used anymore?

JLLeitschuh · 2017-12-14T23:20:31Z

I cleared up my own review comments and I'll merge this if the CI passes.

JLLeitschuh

Looks good to me after making my own changes to the PR. Great work @SamCarlberg I know that this was a significant amount of rewrite work. Thanks for your dedication.

SamCarlberg requested a review from JLLeitschuh May 5, 2017 20:38

JLLeitschuh reviewed May 6, 2017

View reviewed changes

Autodiscover operations at startup. Use annotations for descriptions

aaed76b

Re-enable CompatibilityTest. Fix some backwards compatbility issues. Not really a fan of injecting the Injector into the Operations class. Need to see if there's a better solution

SamCarlberg force-pushed the operation-description-annotations branch from 5f1d0c6 to aaed76b Compare May 6, 2017 21:28

JLLeitschuh reviewed May 9, 2017

View reviewed changes

JLLeitschuh reviewed Jul 24, 2017

View reviewed changes

JLLeitschuh added 2 commits December 14, 2017 17:47

Merge branch 'master' into operation-description-annotations

2f053a9

Cleanup from review comments

0819fce

JLLeitschuh approved these changes Dec 15, 2017

View reviewed changes

JLLeitschuh merged commit 978b4a9 into WPIRoboticsProjects:master Dec 15, 2017

SamCarlberg deleted the operation-description-annotations branch April 19, 2019 16:52

Comments

Conversation

SamCarlberg commented May 5, 2017

Uh oh!

codecov-io commented May 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

SamCarlberg commented May 5, 2017

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!

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!

SamCarlberg commented Jul 21, 2017

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!

JLLeitschuh commented Dec 14, 2017

Uh oh!

JLLeitschuh left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

codecov-io commented May 5, 2017 •

edited

Loading