Add versioning #692

SamCarlberg · 2016-10-26T03:05:21Z

If a save file cannot be loaded, no changes happen to the pipeline and the user is notified. This will happen for any save file, pre-versioning or otherwise.

When saving a loaded project, the original version string will be changed to the version of GRIP that saved it. Pre-versioning save files will also be upgraded to the versioned format and have the current version of GRIP

This depends on #693 for user alerts

If a save file cannot be loaded, no changes happen to the pipeline and the user is notified. This will happen for any save file, pre-versioning or otherwise. When saving a loaded project, the original version string will be changed to the version of GRIP that saved it. Pre-versioning save files will also be upgraded to the versioned format and have the current version of GRIP

codecov-io · 2016-10-26T03:27:27Z

Codecov Report

Merging #692 into master will increase coverage by 0.33%.
The diff coverage is 74.3%.

@@             Coverage Diff              @@
##             master     #692      +/-   ##
============================================
+ Coverage     52.35%   52.68%   +0.33%     
- Complexity     1134     1173      +39     
============================================
  Files           240      249       +9     
  Lines          7724     7909     +185     
  Branches        526      540      +14     
============================================
+ Hits           4044     4167     +123     
- Misses         3494     3545      +51     
- Partials        186      197      +11

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0eb6ce4...51915a9. Read the comment docs.

e.g. trying to generate code with operations that don't support it Post a WarningEvent to the eventbus and an alert dialog will appear. The alert supports markdown formatting for the body text.

Loading a file (.grip or otherwise) should never throw a fatal exception now

JavaFX webview is a mess.

Put the version as an attribute of the project rather than an XML entry.

JLLeitschuh · 2016-10-29T05:59:18Z

Does this really have to use inheritance to solve this problem?
I'm not really a fan of putting a AbstractPipelineEntry in the inheritance hierarchy of Step.
Let's chat about this.

This class does need a serious refactor, though

JLLeitschuh · 2017-03-17T18:55:51Z

ui/src/main/java/edu/wpi/grip/ui/MainWindowController.java

@@ -60,6 +64,7 @@
 /**
 * The Controller for the application window.
 */
+@SuppressWarnings("PMD.GodClass") // temporary


Add an issue to track this or make sure that this isn't in the final version you want to merge.

There's already an issue open for this

JLLeitschuh

Just adding some comments. I don't know what state this is in but I figured I'd provide some feedback for while you are working on it.

JLLeitschuh · 2017-03-20T20:12:45Z

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

+  @Override
+  @SuppressWarnings("PMD")
+  protected void cleanUp() {
+    // Default to NOP


Why not make this a default method?

JLLeitschuh · 2017-03-20T20:15:28Z

core/src/main/java/edu/wpi/grip/core/http/HttpPipelineSwitcher.java

+      baseRequest.setHandled(true);
+    } catch (InvalidSaveException e) {
+      // 406 - Not Acceptable if given an invalid save
+      response.setStatus(HttpServletResponse.SC_NOT_ACCEPTABLE);


Maybe attach a header that contains the message from the exception?

JLLeitschuh · 2017-03-20T20:42:47Z

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

+    if (!inDeserialization) {
+      throw new IllegalStateException(
+          "This method may only be called during project deserialization");
+    }


Why not just make this package private? Or hide it in some other way?

JLLeitschuh · 2017-03-20T20:44:12Z

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

+   * Pool of used IDs.
+   */
+  @VisibleForTesting
+  static class IdPool extends HashMap<Class<? extends PipelineEntry>, Set<String>> {


Guava has a type for this:
https://github.com/google/guava/wiki/NewCollectionTypesExplained#implementations

JLLeitschuh · 2017-03-20T20:46:08Z

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

+      checkDuplicates(operationData, "socket IDs", sockets, Socket::getUid);
+    } finally {
+      operation.cleanUp();
+    }


Not a huge fan of creating an operation when the user hasn't asked the UUID to create one.

Perhaps put this into a test that loads every operation?

JLLeitschuh · 2017-03-20T20:46:55Z

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

+   *
+   * @throws NoSuchElementException if there is no socket with the given UID,
+   */
+  Socket getSocketByUid(String uid) throws NoSuchElementException;


~~Perhaps using the UUID type would be nice.~~

Never mind. I understand now.
Is there some sort of launch time guarantee of being unique?

Part of the contract is that the socket ID has to be unique within its parent

Can you add a unit test that guarantees this contract for all operations?

JLLeitschuh · 2017-03-20T20:47:45Z

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

@@ -24,7 +24,7 @@
 * sockets, and it runs the operation whenever one of the input sockets changes.
 */
 @XStreamAlias(value = "grip:Step")
-public class Step {
+public class Step extends AbstractPipelineEntry {


Why not use composition instead of inheritance?

https://en.wikipedia.org/wiki/Composition_over_inheritance

JLLeitschuh · 2017-03-20T20:54:02Z

core/src/main/java/edu/wpi/grip/core/sockets/Socket.java

+   * the name, view, or even the type changes, the UID has to be constant in order for projects
+   * saved from different versions of GRIP to be compatible.
+   *
+   * @implSpec <strong>This value MAY NOT change</strong>


This value MUST NOT EVER change

Why is this on the socket and not the socket hint?

JLLeitschuh · 2017-03-20T20:57:58Z

core/src/main/java/edu/wpi/grip/core/sources/HttpSource.java

@@ -87,10 +85,10 @@
      GripServer server,
      ContextStore store,
      @Assisted String path) {
-    super(exceptionWitnessFactory);
+    super(makeId(HttpSource.class), exceptionWitnessFactory);


Why do you need this duplicate code in every source?
Why not just have the super constructor pull this information?

JLLeitschuh · 2017-03-20T20:59:36Z

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

+        + "    <connections/>\n"
+        + "    <settings/>\n"
+        + "  </grip:Pipeline>"
+        + "</grip:Project>";


This makes me wish for kotlin multiline strings:
https://kotlinlang.org/docs/reference/basic-types.html#string-literals

SamCarlberg added this to the v2.0.0 milestone Oct 26, 2016

SamCarlberg self-assigned this Oct 26, 2016

SamCarlberg force-pushed the feat/versioning branch from 80a6f26 to f3a0f53 Compare October 26, 2016 03:13

SamCarlberg added 7 commits October 26, 2016 14:32

Add mechanism for alerting the user when they try to do something wrong

f77edf7

e.g. trying to generate code with operations that don't support it Post a WarningEvent to the eventbus and an alert dialog will appear. The alert supports markdown formatting for the body text.

Merge branch feat/warn-user into feat/versioning

faf7514

Use WarningEvent to notify when a save can't be loaded

33e9cc0

Loading a file (.grip or otherwise) should never throw a fatal exception now

Remove markdown from alert body content

033ceca

JavaFX webview is a mess.

Merged feat/warn-user into feat/versioning

ba0b216

Add IDs to Steps and Sources

4f10cd7

Put the version as an attribute of the project rather than an XML entry.

Add tests

a04f012

SamCarlberg added the type: feature label Oct 31, 2016

AustinShalit modified the milestones: v2.0.0, v1.5.0 Nov 7, 2016

SamCarlberg modified the milestones: Future, v1.5.0 Nov 8, 2016

AustinShalit modified the milestones: v2.0.0, Future Dec 25, 2016

SamCarlberg added 2 commits March 16, 2017 22:20

Merge branch master

fe249ad

Fix ClassifierSource

676fa48

SamCarlberg mentioned this pull request Mar 17, 2017

Continuously backup changes and load them when GRIP is launched #822

Open

SamCarlberg added 2 commits March 16, 2017 22:49

Make CameraSource use cleanUp() to do cleanup. Appease checkstyle

46d3886

Temporarily disable GodClass check for MainWindowController

51915a9

This class does need a serious refactor, though

JLLeitschuh reviewed Mar 17, 2017

View reviewed changes

JLLeitschuh reviewed Mar 20, 2017

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add versioning #692

Add versioning #692

SamCarlberg commented Oct 26, 2016 •

edited

Loading

codecov-io commented Oct 26, 2016 •

edited

Loading

JLLeitschuh commented Oct 29, 2016

JLLeitschuh Mar 17, 2017

SamCarlberg Mar 17, 2017

JLLeitschuh left a comment

JLLeitschuh Mar 20, 2017

JLLeitschuh Mar 20, 2017

JLLeitschuh Mar 20, 2017

JLLeitschuh Mar 20, 2017

JLLeitschuh Mar 20, 2017

JLLeitschuh Mar 20, 2017

SamCarlberg Mar 20, 2017

JLLeitschuh Mar 20, 2017

JLLeitschuh Mar 20, 2017

JLLeitschuh Mar 20, 2017

JLLeitschuh Mar 20, 2017

JLLeitschuh Mar 20, 2017

JLLeitschuh Mar 20, 2017

JLLeitschuh Mar 20, 2017

Add versioning #692

Are you sure you want to change the base?

Add versioning #692

Conversation

SamCarlberg commented Oct 26, 2016 • edited Loading

codecov-io commented Oct 26, 2016 • edited Loading

Codecov Report

JLLeitschuh commented Oct 29, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JLLeitschuh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SamCarlberg commented Oct 26, 2016 •

edited

Loading

codecov-io commented Oct 26, 2016 •

edited

Loading